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

feat: webui nav sidebar dropdown text changes #9063

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

JComins000
Copy link
Contributor

@JComins000 JComins000 commented Mar 27, 2024

Description

Changing some surface level text to raise clarity on the sidebar dropdown

Test Plan

Commentary (optional)

image

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

INFENG-547

Copy link

netlify bot commented Mar 27, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit c3106ba
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66049137960f2f0008bd1532

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 40.59%. Comparing base (18dd29e) to head (c3106ba).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9063      +/-   ##
==========================================
- Coverage   47.16%   40.59%   -6.57%     
==========================================
  Files        1150      829     -321     
  Lines      141674   102498   -39176     
  Branches     2415     2415              
==========================================
- Hits        66814    41614   -25200     
+ Misses      74670    60694   -13976     
  Partials      190      190              
Flag Coverage Δ
harness ?
web 38.96% <66.66%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
webui/react/src/components/ThemeToggle.tsx 100.00% <100.00%> (ø)
webui/react/src/components/UserSettings.tsx 83.84% <100.00%> (ø)
webui/react/src/components/NavigationSideBar.tsx 0.00% <0.00%> (ø)

... and 321 files with indirect coverage changes

Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

The code looks good and IMO the label changes are good. Not 100% sure about Day Theme and Night Theme though, I think the standard around that is still Dark Mode and Light Mode.

Screenshot 2024-03-27 at 2 22 13 PM

Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

approved pending ci pass

@JComins000
Copy link
Contributor Author

The code looks good and IMO the label changes are good. Not 100% sure about Day Theme and Night Theme though, I think the standard around that is still Dark Mode and Light Mode.

Screenshot 2024-03-27 at 2 22 13 PM

I mostly need it to say THEME on it. im fine with light and dark if you feel strongly

@JComins000 JComins000 force-pushed the jcom/INFENG-547/sidebar-dropdown-text-changes branch from 3f2c4a9 to c3106ba Compare March 27, 2024 21:35
@JComins000 JComins000 merged commit d9e1088 into main Mar 27, 2024
68 of 81 checks passed
@JComins000 JComins000 deleted the jcom/INFENG-547/sidebar-dropdown-text-changes branch March 27, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants