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

Refactor tab-size to reduce conflicts #6480

Merged
merged 10 commits into from Apr 10, 2023
Merged

Refactor tab-size to reduce conflicts #6480

merged 10 commits into from Apr 10, 2023

Conversation

@kidonng kidonng added the bug label Apr 9, 2023
@kidonng kidonng requested a review from fregante April 9, 2023 12:28
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

See the "test" section in #4833

I see at least one issue currently:

  • tab-size.css is setting tab-size but not --tab-size, which means show-whitespace will be broken

I think this only affects inlined code: Screenshot from #4833
Screenshot 10

source/features/tab-size.css Outdated Show resolved Hide resolved
source/features/tab-size.css Outdated Show resolved Hide resolved
@fregante fregante changed the title Set tab size less aggressively Refactor tab-size to reduce conflicts Apr 9, 2023
@fregante
Copy link
Member

fregante commented Apr 9, 2023

I hope to merge this by tomorrow so that I can release 23.4.10

@fregante fregante marked this pull request as draft April 9, 2023 14:54
@kidonng kidonng marked this pull request as ready for review April 9, 2023 15:24
source/features/tab-size.css Outdated Show resolved Hide resolved
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

All aboard! Thanks for looking into this!

source/features/show-whitespace.css Show resolved Hide resolved
source/features/tab-size.css Outdated Show resolved Hide resolved
@fregante fregante self-assigned this Apr 10, 2023
@fregante fregante marked this pull request as draft April 10, 2023 07:10
@fregante fregante removed their assignment Apr 10, 2023
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

I think we're good now, there was an issue with the inconsistency between --tab-size and tab-size in some situations, so I think this should make sure it doesn't happen anymore.

* {tab-size: var(--tab-size)} would probably be the only way to ensure this, but I don’t think it's necessary.

Note

GitHub currently has a bug where the first tab in code embeds is trimmed. This is unrelated to RG (see the tabs below)

Screenshot 11

Comment on lines +10 to 14
:root,
.tab-size[data-tab-size='8'] {
--tab-size: 4;
tab-size: var(--tab-size);
}
Copy link
Member

Choose a reason for hiding this comment

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

The tab-size feature overrides the tab size. This one single selector overrides it. I think this is the most consistent situation we got so far

Copy link
Member Author

@kidonng kidonng Apr 10, 2023

Choose a reason for hiding this comment

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

Sorry if my changes have confused you 😢

Could have just reverted eb6f729 (#6480) if you feel the former version is better

Now this selector isn't specific enough (lacking .comment-body) 🤔

I opened a new pull request to fix this

Copy link
Member

Choose a reason for hiding this comment

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

Heh you're right I didn't even notice that I just undid your commit 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fine, I guess I'm just bad at expressing my intent (and tend to complicate things) 😂

@fregante fregante marked this pull request as ready for review April 10, 2023 07:42
@fregante fregante merged commit 54dd71e into main Apr 10, 2023
9 checks passed
@fregante fregante deleted the tab-size branch April 10, 2023 07:42
@fregante
Copy link
Member

phew finally 😅

Comment on lines +44 to +46
[data-tab-size='7'] {
--tab-size: 7;
}
Copy link
Member Author

@kidonng kidonng Apr 10, 2023

Choose a reason for hiding this comment

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

Does this appear anywhere? Preferences only have 9 options: 1-6, 8, 10, 12. Not all 12 steps are available.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, this was more like (unnecessary) future-proofing 😂

Screenshot

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add this to lint (to-do)

@kidonng kidonng mentioned this pull request Apr 10, 2023
kidonng added a commit that referenced this pull request Apr 10, 2023
These does not exist in GitHub preferences
#6480 (comment)
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.

File previews broken due to lack of respect for user's tab-size tab-size broken in new code view
3 participants