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

🪟 🔧 Include module name in CSS class names #21746

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

timroes
Copy link
Collaborator

@timroes timroes commented Jan 23, 2023

What

Makes CSS class names be generated in a format to contain the original file name. See the screenshot (except the highlighted row, which is a styled component):

screenshot-20230123-213530

@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 23, 2023
@timroes timroes enabled auto-merge (squash) January 23, 2023 20:53
Copy link
Contributor

@ambirdsall ambirdsall left a comment

Choose a reason for hiding this comment

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

👍 Much nicer. ✨ 🎸 🔥

One issue this highlights (to be clear, an issue that is out of scope for this PR): the .module.scss filename convention is, correctly, only being parsed as a .scss extension, so all the generated classes have a boilerplate -module in the middle, which I think hurts readability:

generated class name wouldn't it be nice, tho
Card-module__header__ac1808 Card__header__ac1808
ConnectionStatusPage-module__title__ed6618. ConnectionStatusPage__title__ed6618

Since IIUC no build mechanism relies on that convention, perhaps a change is in order?

@timroes timroes merged commit 072717e into master Jan 23, 2023
@timroes timroes deleted the tim/more-detailed-css-classes branch January 23, 2023 23:59
@timroes
Copy link
Collaborator Author

timroes commented Jan 24, 2023

@ambirdsall sorry this PR was on auto merge so got merged with the approval automatically.

The .module.scss is actually needed for the build system to detect it as a CSS module and make the classnames be importable. I could though have specified a function for this property that calculates the name, but that would also require us to do the content hashing etc. ourselves. I found the minor disadvantage of having module in the classnames would outweigh adding 5-10 more lines of classnames calculation in the build script. But if we feel that the module is annoying too much in the class name, I'm open for that path as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants