-
Notifications
You must be signed in to change notification settings - Fork 21
Fix vertical alignment in topmost collapseUnchanged
and collapseRanges
bars
#2911
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
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Results 1 files ±0 1 suites ±0 8m 30s ⏱️ -30s For more details on these errors, see this check. Results for commit 373e664. ± Comparison against base commit 5b69f23. ♻️ This comment has been updated with latest results. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
2e30b33
to
9f832f7
Compare
Bundle ReportChanges will decrease total bundle size by 1.67kB (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
|
@@ -240,6 +241,7 @@ const BASE_STYLE = { | |||
|
|||
'& .cm-collapsedRangesInner': { | |||
borderTop: 'none', | |||
paddingTop: '6px', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this magic number come from 🤔 Any way to make this a bit more obvious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we don't document things like 1px, 0.5 rem etc. since they directly map to Tailwind units. Anything outside common values is worth looking at - either we change to use the standard ones, or we document well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sure, magic number comes from CodeMirror itself where default padding is 5px, and I am making it 6.
I was thinking myself about how to improve this, maybe use CSS variables and calc
. The problem is that CodeMirror does not use Tailwind, the values are hardcoded either in default styles or in default theme (which we override), and CodeMirror is kind of it's own separate thing, a subproject inside our project. Can't wait for a chance to extract it into a separate Ember addon and repository, so it doesn't pollute ours with stuff like this.
Will see what I can do with this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added note re: magic number
9f832f7
to
a93a137
Compare
a93a137
to
373e664
Compare
Brief
This fixes vertical text alignment in topmost
collapseUnchanged
andcollapseRanges
bars.Details
Since our file headers have box-shadow and topmost bars lack the top border — an extra 1px
padding-top
needs to be added for proper alignment.After
Before
Checklist
[percy]
in the message to trigger)