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

[WEB-3702] Reinstate TW layer directives, remove JSX CSS imports #337

Merged
merged 12 commits into from
Apr 29, 2024

Conversation

jamiehenson
Copy link
Member

@jamiehenson jamiehenson commented Apr 25, 2024

Jira Ticket Link / Motivation

https://ably.atlassian.net/browse/WEB-3702

Summary of changes

As part of the Ably UI rewrite to incorporate Storybook and ditch Rails, the Tailwind "layer" directives were removed to solve issues with rendering components directly in Storybook. Turns out, we depend upon layer directives much more than I initially thought, and removing them means that CSS selector prioritisation is all out of whack in apps that use ably-ui classes, leading to large visual regressions.

This PR reinstates the layer directives to how they were before the Storybook rewrite, but with a shifted compromise - dedicated component CSS files must now be imported within the global CSS file (or wherever the tailwindcss/base et al imports are), instead of imported within the component themselves (example here is within .storybook/styles.css). I think this is a suitable compromise.

Additional changes here:

  • cleanup of additional "important" styles (no longer needed with the above fix)
  • remove explicit postcss deps from package.json (vite handles this, we don't need separate deps)
  • add the option to bypass height constraints on Expander

How do you manually test this?

Use the latest ably-ui 14 dev package in Voltaire, navigate around, check that TW utility overrides on Ably UI classes works correctly. An example of this is a text-white naturally overriding the color in ui-text-p1, instead of having to elevate text-white to !text-white.

Reviewer Tasks (optional)

Merge/Deploy Checklist

  • Written automated tests for implemented features/fixed bugs
  • Rebased and squashed commits
  • Commits have clear descriptions of their changes
  • Checked for any performance regressions

Frontend Checklist

  • No frontend changes in this PR
  • Added before/after screenshots for changes
  • Tested on different platforms/browsers with Browserstack
  • Compared with the initial design / our brand guidelines
  • Checked the code for accessibility issues (VoiceOver User Guide)?

Copy link
Member

@kennethkalmer kennethkalmer left a comment

Choose a reason for hiding this comment

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

Hiding whitespace makes the PR a breeze to review :)

It looks good to me, although I only pulled down the branch and tested Storybook to be sure. I'll defer to @aleksandar-r for the Voltaire testing and checks.

Do we need to update the README about how to load the styles in Voltaire or the website?

@jamiehenson
Copy link
Member Author

There's already kinda an indicator in the README here that component styles should be loaded outside of the component (https://github.com/ably/ably-ui/blob/main/README.md?plain=1#L101-L111), but I'll make a note to update the voltaire readme when doing other QA stuff

Copy link
Member

@kennethkalmer kennethkalmer left a comment

Choose a reason for hiding this comment

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

@jamiehenson jamiehenson merged commit 18c0ae1 into main Apr 29, 2024
6 checks passed
@jamiehenson jamiehenson deleted the compare-page-iterations branch April 29, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants