Skip to content

Conversation

@severo
Copy link
Contributor

@severo severo commented Mar 27, 2025

fixes #165

Sorry for the review @platypii, it's a very big PR.

  • empty app.css and replace it with small CSS module files (the global styles remain, as they cannot be part of CSS modules; I moved them to global.css, a new file that I export in the npm package)
  • give the library client the ability to override the styles
  • replace class name with CSS selectors, based on improved semantics and aria attributes.
  • [maybe] split the CSS modules in two classes: .unstyled and .theme - honestly, I think it's not necessary, and that we want a good default theme, and the ability to tweak it at the margin (see point 2), but not to apply a different theme.
  • reorganize the files (see following proposal - it could perfectly be in another PR)

I have a proposal for this PR, @platypii:

I think we could reorganize the files a bit, like I did in hightable, loosely following https://www.joshwcomeau.com/react/file-structure/:

  • move the files from components/viewers to components/ (no semantic hierarchy)
  • move every component file to its own subdirectory (App.tsx -> App/App.tsx)
  • move the styles (CSS modules) to these subdirectories (styles/Json.module.css => components/Json/Json.module.css)
  • (maybe) move the tests to these subdirectories (test/Layout.test.tsx => components/Layout/Layout.test.tsx) - it requires tweaking the build process to ignore these files.

It would help find the files quickly, and it would group the styles and components together (this PR will create many CSS module files).

@severo severo changed the title 165 use css modules Use CSS modules Mar 27, 2025
@severo severo force-pushed the 165-use-css-modules branch from 4329c70 to 0fde1db Compare March 28, 2025 09:34
@severo severo force-pushed the 165-use-css-modules branch from ceae0ec to 76a1f29 Compare March 28, 2025 09:48
@severo severo requested a review from platypii March 28, 2025 21:56
@severo severo marked this pull request as ready for review March 28, 2025 21:56
Copy link
Contributor

@platypii platypii left a comment

Choose a reason for hiding this comment

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

Looks good. Splitting out smaller components seems like a good thing. The two that feel a little weird are Center and VisuallyHidden which feel more like styles than components. It good for now though. 👍

@severo
Copy link
Contributor Author

severo commented Mar 31, 2025

Re VisuallyHidden: it's a common component in any library, and is the current best practice to provide an accessible text without showing it visually (it's more accessible than aria-label). I agree that it's related to the style though.

For Center... I don't like it too much, either. Maybe I'll revisit if I find a better way.

@severo severo merged commit dcb467a into master Mar 31, 2025
4 checks passed
@severo severo deleted the 165-use-css-modules branch March 31, 2025 08:05
@severo
Copy link
Contributor Author

severo commented Mar 31, 2025

Also @platypii: do you have an opinion on the code structure, ie:

Current: three times the same hierarchy in:

  • src/components/Component.tsx
  • src/styles/Component.module.css
  • test/Component.test.tsx

versus the proposal:

  • src/components/Component/Component.tsx
  • src/components/Component/Component.module.css
  • src/components/Component/Component.test.tsx

I created #189 to discuss it.

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.

Use CSS modules to define a style for the components

3 participants