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

Feature/1028 #1029

Closed
wants to merge 2 commits into from
Closed

Feature/1028 #1029

wants to merge 2 commits into from

Conversation

fraincs
Copy link
Collaborator

@fraincs fraincs commented Oct 3, 2022

Reworked the css declarations using first-child as it could be unsafe in SSR. This has a downside as first-of-type(which is what Next is suggesting using) only works with elements and not classes. Rendering the CSS more flimsy, this is not an issue as we control the elements in Orbit, but something to be aware of if we ever change the DOM of certain components.

@netlify
Copy link

netlify bot commented Oct 3, 2022

Deploy Preview for sg-orbit ready!

Name Link
🔨 Latest commit 674dee6
🔍 Latest deploy log https://app.netlify.com/sites/sg-orbit/deploys/633b34aa54e7020008f64a0e
😎 Deploy Preview https://deploy-preview-1029--sg-orbit.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Oct 3, 2022

Deploy Preview for sg-storybook ready!

Name Link
🔨 Latest commit 674dee6
🔍 Latest deploy log https://app.netlify.com/sites/sg-storybook/deploys/633b34aa3b5d56000968b7c6
😎 Deploy Preview https://deploy-preview-1029--sg-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@patricklafrance
Copy link
Member

Very cool that you are handling this issue.

I wonder thought how safer it is by specifying the type like this instead of classes? I feel less safe with this changes than before haha.

@fraincs
Copy link
Collaborator Author

fraincs commented Oct 4, 2022

Very cool that you are handling this issue.

I wonder thought how safer it is by specifying the type like this instead of classes? I feel less safe with this changes than before haha.

I understand your concern, that said, the *-of-type selectors are suppose to be for elements and not classes, it kinda works although not officially supported (https://developer.mozilla.org/docs/Web/CSS/:last-of-type), which means that it could break anytime. Doing a class:last-of-type will select the last element with the class, which might be exactly what we want but since it's not to spec, I'm a little frisky to use and hope browsers does not change the way they treat this case.

@fraincs
Copy link
Collaborator Author

fraincs commented Oct 4, 2022

@patricklafrance looking at this it seems it's more a Storybook / Emotion issue (storybookjs/storybook#18103) I don't see why compiled CSS would be affected by this.

@patricklafrance
Copy link
Member

Ahhh, great find @fraincs

It's not an Orbit issue then.

@fraincs fraincs closed this Oct 4, 2022
@fraincs fraincs deleted the feature/1028 branch March 13, 2023 17:43
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.

None yet

2 participants