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.

Sign up for GitHub

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

Jump to bottom

web: lower .card priority #46274

Merged
merged 2 commits into from
Jan 10, 2023
Merged

web: lower .card priority #46274

merged 2 commits into from
Jan 10, 2023

Conversation

valerybugakov
Copy link
Member

@valerybugakov valerybugakov commented Jan 10, 2023 •
edited

Context

To address the root cause, we need to invest more time in restructuring Wildcard exports, as described in the linked issue.

Restructure exports in packages with CSS modules to enable tree-shaking without relying on the sideEffects property.

Test plan

  1. sg start web-standalone-prod
  2. Check that the toast is styled correctly.

App preview:

Check out the client app preview documentation to learn more.

@valerybugakov valerybugakov self-assigned this Jan 10, 2023
@cla-bot cla-bot bot added the cla-signed label Jan 10, 2023
@github-actions github-actions bot added the team/code-exploration Issues owned by the Code Exploration team label Jan 10, 2023
@sg-e2e-regression-test-bob
Copy link

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (+0.02 kb) 0.00% (+0.02 kb) 0.00% (0.00 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits dcf8246 and 6bf0cf8 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@valerybugakov valerybugakov marked this pull request as ready for review January 10, 2023 12:52
@valerybugakov valerybugakov requested review from leonore, sqs and a team January 10, 2023 12:53
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Jan 10, 2023 •
edited

Codenotify: Notifying subscribers in OWNERS files for diff a785e20...dcf8246.

Notify File(s)
@sourcegraph/code-exploration-devs client/wildcard/src/components/Card/Card.module.scss
@vovakulikov client/wildcard/src/components/Card/Card.module.scss

leonore
leonore reviewed Jan 10, 2023
View reviewed changes
Copy link
Contributor

@leonore leonore left a comment •
edited

Choose a reason for hiding this comment

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

I have no context/experience on .card usage so don't want to blindly approve but thank you for taking a look 😄

client/wildcard/src/components/Card/Card.module.scss
@@ -1,4 +1,6 @@
.card {
// :where() is used to fix the CSS ordering issue with the Toast component
// See https: //github.com/sourcegraph/sourcegraph/issues/42217
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// See https: //github.com/sourcegraph/sourcegraph/issues/42217
// See https://github.com/sourcegraph/sourcegraph/issues/42217

philipp-spiess
philipp-spiess approved these changes Jan 10, 2023
View reviewed changes
Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

TIL about :where and it always having a specificity of 0 😱

https://developer.mozilla.org/en-US/docs/Web/CSS/:where

@philipp-spiess
Copy link
Contributor

@valerybugakov Chromatic shows a regression for border-radius on some components

@philipp-spiess
Copy link
Contributor

Yeah it looks like some <button> styling takes presedence over the Card styling which is not what we want :/

Screenshot 2023-01-10 at 15 15 12

@philipp-spiess
Copy link
Contributor

I’m fixing this by adding a !important for now. I know, I know but when a card is used you always want to use the border radius anyways 😅. Let's chat tomorrow for how this could be better fixed 🤔 We could also bump the specificity of the .toast class instead?

For now I'll merge this when tests are green so we have unblocked main.

@philipp-spiess philipp-spiess merged commit 0f69db9 into main Jan 10, 2023
@philipp-spiess philipp-spiess deleted the vb/card-css-order branch January 10, 2023 14:45
limitedmage
limitedmage reviewed Jan 10, 2023
View reviewed changes
client/wildcard/src/components/Card/Card.module.scss
@@ -16,7 +18,7 @@
border-width: 1px;
border-style: solid;
border-color: var(--card-border-color);
border-radius: var(--card-border-radius);
border-radius: var(--card-border-radius) !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is !important required here?

Copy link
Contributor

Choose a reason for hiding this comment

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

#46274 (comment) We have some conflicts with CSS resets otherwise it seems. In the case above, a button { border-radius: 0 } rule from Chromatic regressed. I wanted to ship this PR quickly though and will follow-up with @valerybugakov tomorrow on how to improve this but the issue was quite noticeable for some people on S2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Can we add a comment here explaining that? I don't think we should have undocumented !importants.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @philipp-spiess!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@limitedmage limitedmage limitedmage left review comments

@philipp-spiess philipp-spiess philipp-spiess approved these changes

@leonore leonore leonore left review comments

@sqs sqs Awaiting requested review from sqs

Assignees

@valerybugakov valerybugakov

Labels
cla-signed css-order-in-production team/code-exploration Issues owned by the Code Exploration team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@valerybugakov @sg-e2e-regression-test-bob @sourcegraph-bot @philipp-spiess @limitedmage @leonore