simularium / simularium-website

Front end website for the Simularium project, includes the Simularium viewer
https://simularium.allencell.org
Apache License 2.0
6 stars 3 forks source link

fix: organize colors, modal buttons #473

Closed interim17 closed 5 months ago

interim17 commented 6 months ago

Time to review

Medium: mostly reorganization, pruning of unused variables, renaming, etc

Problem

A maintenance PR to organize colors modal button styles, and make some style fixes on modal buttons. Preliminary work to some updates to CustomModal.

With the exception of modal buttons, these changes should not affect current styling anywhere else in the application.

To review:

Inspect the website for inadvertent changes, and inspect the three modals: share trajectory, version info, upload file (screenshots below).

Almost all changes are trivial except for colors.css. In colors I have tried hard to confirm that all removed variables were unused, and no styles were unintentionally changed by reorganization/pruning. Missing something here and messing with other styles unintentionally is the primary concern in review

Theme and styling:

General color maintenance:

removed unused css vars:

--dark-blue-grey
--viewer-btn-border-on
--border-gray
--background-dim-gray
--background-dark-gray
--light-theme-button-purple
--light-theme-button-light-gray
--light-theme-input-border-highlight
--dark-theme-btn-disabled-color
--dark-theme-btn-primary-text
--dark-theme-btn-disabled-bg
--dark-theme-slider-color

renamed/reused: (using primary and secondary now to match UX usage)

--light-theme-btn-default-bg
--light-theme-btn-default-color
--light-theme-btn-default-border
--light-theme-btn-default-hover-bg
--light-theme-btn-default-hover-color
--light-theme-btn-default-hover-border
--light-theme-btn-default-disabled-bg
--light-theme-btn-default-disabled-color
--light-theme-btn-default-disabled-border

Screenshots (optional):

Old:

Screenshot 2024-03-06 at 5 11 55 PM

New:

Screenshot 2024-03-06 at 5 31 55 PM

Old:

Screenshot 2024-03-06 at 5 10 16 PM

New:

Screenshot 2024-03-06 at 5 10 10 PM

Old:

Screenshot 2024-03-06 at 5 09 38 PM

New:

Screenshot 2024-03-06 at 5 25 20 PM

Old:

Screenshot 2024-03-06 at 5 08 11 PM

New:

Screenshot 2024-03-06 at 5 07 36 PM
github-actions[bot] commented 6 months ago

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟡 Statements 73.27% 603/823
🟡 Branches 68.38% 80/117
🔴 Functions 40.31% 79/196
🟡 Lines 71.73% 538/750

Test suite run success

99 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from eba4d9744a73d208bb7034921d6ec6e1c2ac6201

meganrm commented 6 months ago

When I compared these to the design, the x in the modal looks farther away from the right edge in your build. I think this is probably the default antd positioning, but just FYI

meganrm commented 6 months ago

When I compared these to the design, the x in the modal looks farther away from the right edge in your build. I think this is probably the default antd positioning, but just FYI

suggestion: add justify-content: center; to .style__modal--vTNFd .ant-modal-close-x

meganrm commented 6 months ago

All the secondary buttons lose their outline on focus:

Screenshot 2024-03-07 at 2 48 04 PM

Ooops, I had a type in my primary button focus selector, it was actually targeting focused secondary buttons. I think that fixed it.

interim17 commented 6 months ago

When I compared these to the design, the x in the modal looks farther away from the right edge in your build. I think this is probably the default antd positioning, but just FYI

You're right, I have a fix for that lined up in my next set of changes. Used your justify-content suggestion though, can see about the other fix later.