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

Fix conflict-marker spacing #4128

Merged
merged 5 commits into from
Apr 2, 2021
Merged

Fix conflict-marker spacing #4128

merged 5 commits into from
Apr 2, 2021

Conversation

cheap-glitch
Copy link
Member

Fixes #4117

Test URLs

https://github.com/eslint/eslint/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+comments+

Screenshot

With align-issue-labels enabled (no change):

Before

image

After

image

With align-issue-labels disabled:

Before

image

After

image

@yakov116 yakov116 added the bug label Mar 18, 2021
Co-authored-by: yakov116 <16872793+yakov116@users.noreply.github.com>
@fregante
Copy link
Member

fregante commented Mar 18, 2021

I didn't notice you had already opened a PR 😅 check out my comment in the original issue, this probably isn't the right solution, especially because we already have a selector that does the exact same thing

@fregante fregante marked this pull request as draft March 19, 2021 22:01
Copy link
Member Author

@cheap-glitch cheap-glitch left a comment

Choose a reason for hiding this comment

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

Both align-issue-labels and conflict-marker enabled:

image

Just align-issue-labels:

image

Just conflict-marker:

image

@cheap-glitch cheap-glitch marked this pull request as ready for review March 30, 2021 09:57
.rgh-align-issue-labels .js-issue-row .d-inline-block.mr-1 { /* Build status */
padding-right: 4px; /* Space before the build status */
padding-left: 4px;
Copy link
Member

Choose a reason for hiding this comment

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

Why yes both padding and margin though? Is this needed at all? Should the item just have a single mx-2 class?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a pure CSS feature though, is it worth it to add JS just for this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah indeed I thought that the spacing here was exactly due to the other feature changed in this PR, but instead it's a native icon.

@fregante fregante changed the title Fix conflict-marker spacing when align-issue-labels is disabled Fix conflict-marker spacing Apr 2, 2021
@fregante fregante merged commit 1225061 into refined-github:main Apr 2, 2021
@fregante
Copy link
Member

fregante commented Apr 2, 2021

I changed the title because the fix is mostly about doing things right rather than having code that depends on each other. If one day GitHub drops align-issue-labels then the alignment also breaks.

This has the welcome side effect that it helps people who disable features

@cheap-glitch cheap-glitch deleted the fix-conflict-icon-spacing branch April 2, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

conflict-marker icon is different from the screenshot.
3 participants