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

DOP-4731: Toggle dark mode for ListTable #1121

Merged
merged 12 commits into from
Jun 11, 2024
Merged

DOP-4731: Toggle dark mode for ListTable #1121

merged 12 commits into from
Jun 11, 2024

Conversation

rayangler
Copy link
Collaborator

@rayangler rayangler commented Jun 7, 2024

Stories/Links:

DOP-4731

Current Behavior:

Server docs - tables that include zebra striping
Server docs - tables that include stub columns
Server docs - tables that include stub columns and zebra striping (zebra stripe styles take precedence)
Server docs - tables that are also in callouts

Staging Links:

Server docs - tables that include zebra striping
Server docs - tables that include stub columns
Server docs - tables that include stub columns and zebra striping (zebra stripe styles should take precedence, like in prod)
Server docs - tables that are also in callouts

Notes:

As discussed in planning, upgrading the LG Table is out of scope for this ticket due to a lot of breaking changes and restructuring we'd need to do to accommodate the latest version of the table component. For this reason, I went ahead and overwrote the dark mode colors for just the rows for the table, and it matches the site theme well now. Otherwise, the style would not match the current dark mode theme:

Screen Shot 2024-06-07 at 2 18 06 PM

We should consider upgrading the component as part of DOP-3614 relatively soon though.

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

@rayangler rayangler marked this pull request as ready for review June 7, 2024 18:22
Copy link
Collaborator

@mayaraman19 mayaraman19 left a comment

Choose a reason for hiding this comment

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

Thank you for doing this, the zebra stripes look great! I have a couple comments:

  • we probably should have given this more visibility, but offline we decided that our convention for changing CSS styles in dark mode would be to use CSS variables that are passed in via the style attribute - check out this change in one of Caesar's PRs, hopefully that makes the convention more clear or also reach out if you have questions ofc
  • Also, maybe this is out of the scope of this PR, but the font for some tables/parts of tables changes in Dark Mode? The table under this section for example

Copy link
Collaborator

@seungpark seungpark left a comment

Choose a reason for hiding this comment

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

looks great! just a minor question below concerning alternating rows

src/components/ListTable.js Outdated Show resolved Hide resolved
@rayangler
Copy link
Collaborator Author

we probably should have given this more visibility, but offline we decided that our convention for changing CSS styles in dark mode would be to use CSS variables that are passed in via the style attribute - check out this change in one of Caesar's PRs, hopefully that makes the convention more clear or also reach out if you have questions ofc

Thanks for the heads up! Since we're overriding an internal LG style rather than a CSS style of our own, this might lead to us needing to redefine LG's light mode colors as well with this pattern. I can give it a try though

Also, maybe this is out of the scope of this PR, but the font for some tables/parts of tables changes in Dark Mode? The table under this section for example

Thanks for flagging this! I hadn't even realized it was a different font. Seems like "Euclid Circular A" isn't included in the font family for some reason for dark mode... I'll look into this.

@rayangler
Copy link
Collaborator Author

Also wanted to flag that I just noticed really awkward styling with stub columns (see the 2nd and 3rd tables in this page). I'll spend some time looking into this

@rayangler
Copy link
Collaborator Author

@mayaraman19 I've gone ahead and addressed the 2 things you mentioned. The style prop does make it a bit awkward with needing to redefine certain default styles we're overriding, but hopefully it still looks sane. Let me know what you think!

I've also gone ahead and updated staging links with a couple more examples as well

Copy link
Collaborator

@seungpark seungpark left a comment

Choose a reason for hiding this comment

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

LGTM ! great catches on the stub and font families!

Copy link
Collaborator

@mayaraman19 mayaraman19 left a comment

Choose a reason for hiding this comment

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

👍 thank you for doing this!

@rayangler rayangler merged commit 067d977 into main Jun 11, 2024
2 checks passed
@rayangler rayangler deleted the DOP-4731 branch June 11, 2024 19:24
seungpark added a commit that referenced this pull request Jul 11, 2024
seungpark added a commit that referenced this pull request Jul 11, 2024
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.

3 participants