Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(theme-classic): toggle code wrap button #7036
feat(theme-classic): toggle code wrap button #7036
Changes from 8 commits
f37f1b9
7d483b4
f6bae8b
dab2f96
6d57368
97e6922
82966a2
4b19c7a
5a14be5
3b43394
579b8aa
e8d2841
0464aa9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'd like the button to not decide where it will be rendered: that's the responsibility of the parent
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.
I don't like it either, I was thinking of grouping these buttons into one new div container, but it's not that easy because we would have to somehow get background of the current Prism color theme (need it for button background, not its container). Therefore
inherit
value won't work here, so maybe we dynamically set two new CSS vars, like as--prism-background
and--prism-color
? Will that be acceptable solution?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.
Maybe this works?
usePrismTheme().plain.backgroundColor
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.
I mean, in the case of button container, we need to set the
style
attribute for each button inside to set the proper background. I think it's kind of redundant overhead, which we can avoid by dynamically creating two CSS vars that also can re-use in other places where thestyle
attribute is currently used.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.
@lex111 FWIW, Prism also inlines their styles, so it won't be too harmful
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.
Just wouldn't want to overuse it, especially if there's a pretty appropriate alternative solution. Beside that, using inline styles would require creating one specific prop
style
for each button component, which can also be avoided.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.
Is this rule still necessary?
Looks to me it's not useful anymore => removing it and things keep working?
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.
maybe we want to use the same size for all buttongroup icons? and introduce anv env variable to customize?
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.
It doesn't work that way, the proportions of both icons are different, so if we use the same sizing for them, the wordWrap button will look smaller than the copyButton icon:
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.
we can keep it this way for now, but I guess the wordWrap svg could also be cropped somehow to take full space