Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix looker3d controls styles in light theme #2236

Merged
merged 3 commits into from
Nov 5, 2022

Conversation

imanjra
Copy link
Contributor

@imanjra imanjra commented Nov 4, 2022

What changes are proposed in this pull request?

Improve Looker3D controls styles to make it consistent with looker 2D controls styles

How is this patch tested? If it is not, please explain why.

Tested visually in both light and dark theme

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

@imanjra imanjra requested a review from ritch November 4, 2022 00:59
@benjaminpkane
Copy link
Contributor

Looks good. For consistency, can the grey border be removed from the Looker3d controls row? It also looks like the the icons are smaller in Looker3d. And there is no animation on hover, but that is a smaller detail.
Screen Shot 2022-11-04 at 9 58 07 AM

@imanjra imanjra force-pushed the bugfix/looker3d-controls-style branch from 49eafa6 to 115fdd8 Compare November 4, 2022 18:20
@imanjra imanjra force-pushed the bugfix/looker3d-controls-style branch from 115fdd8 to 0781cad Compare November 4, 2022 20:51
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imanjra can you address these related items before merging?

  1. Clicking the ? icon in Looker3D a second time does not close the help menu (it immediately reopens)

  2. This Looker3D component needs to respect light theme

Screen Shot 2022-11-04 at 8 05 46 PM

  1. The close modal buttons are inconsistent in light theme (I think dark color makes sense)

Screen Shot 2022-11-04 at 8 09 12 PM

Screen Shot 2022-11-04 at 8 08 54 PM

Screen Shot 2022-11-04 at 8 05 36 PM

  1. This overlay text should be white in both dark and light theme:

Screen Shot 2022-11-04 at 8 08 39 PM

Screen Shot 2022-11-04 at 10 28 34 PM

  1. Remove shadow from Looker3D's toolbar for consistency with Looker2D's toolbar:

Screen Shot 2022-11-04 at 10 25 33 PM

Screen Shot 2022-11-04 at 10 25 29 PM

  1. Use same darker background for JSON viewer that is used in help menu (too difficult to read the JSON viewer currently). Also I think the transparency is too much right now. Better to make it more opaque so the modal text is easier to read:

Screen Shot 2022-11-04 at 10 30 01 PM

Screen Shot 2022-11-04 at 10 29 52 PM

@imanjra imanjra requested a review from brimoor November 5, 2022 04:51
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @imanjra!

@brimoor brimoor merged commit d3f155d into develop Nov 5, 2022
@brimoor brimoor deleted the bugfix/looker3d-controls-style branch November 5, 2022 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants