Choices text box: make sure all choice tokens are readable #1982
Conversation
hexaltation
left a comment
hexaltation
left a comment
There was a problem hiding this comment.
LGTM
Thanks Mr. @manuhabitela
fflorent
moved this from Needs Internal Feedback
to Needs feedback
in French administration Board
dsagal
left a comment
dsagal
left a comment
There was a problem hiding this comment.
Thanks! This works, but I think we can reuse code better. Would you mind trying to get rid of getRenderFillColor and getRenderTextColor entirely, and replacing their only usage (in ChoiceListEditor.ts) with a call to choiceToken() instead? (Since the code for renderToken in ChoiceListEditor.ts seems to duplicate the code in choiceToken, which already deals correctly with colors.) It seems possible, perhaps with a little tweaking here and there, but if it's not looking like an improvement or if you get bogged down, let me know.
This is a follow-up to gristlabs#1529, where we made sure that choice tokens with custom bg color but no custom fg color would not become unreadable when switching from light to dark themes. The tokens that were rendered when editing a text cell did not benefit from this fix. So while the tokens stayed readable when looking at them, they could go back to being unreadable when editing them after double-clicking the cell (with automatic light text in dark theme, on a light custom bg, for example). This fixes that use-case.
manuhabitela
force-pushed
the
fix-choices-editor-colors
branch
from
1b8a9e4 to
a8b089c
Compare
|
Thanks Dmitry. I had a try at it in the last commit. What do you think? |
dsagal
left a comment
dsagal
left a comment
There was a problem hiding this comment.
Perfect, this is what I was hoping for! I just have a very tiny nitpick.
app/client/widgets/ChoiceToken.ts
Outdated
| export function choiceTokenDomArgs( | ||
| label: DomElementArg, | ||
| options: IChoiceTokenOptions, | ||
| ...args: DomElementArg[] |
There was a problem hiding this comment.
I think it's better to omit this ...args argument -- the caller can simply add extra args to the DOM after the result of this call, e.g.
cssChoiceToken(choiceTokenDomArgs(label, options), ...args);
There was a problem hiding this comment.
Good catch, woops, thanks.
manuhabitela
force-pushed
the
fix-choices-editor-colors
branch
from
a8b089c to
426dc3f
Compare
dsagal
left a comment
dsagal
left a comment
There was a problem hiding this comment.
Perfect, thanks! It's my favorite kind of change: fixing bugs by reducing the amount of code.
github-project-automation
bot
moved this from Needs feedback
to Done
in French administration Board
3 participants
Context
This is a follow-up to #1529, where we made sure that choice tokens with custom bg color but no custom fg color would not become unreadable when switching from light to dark themes.
The tokens that were rendered when editing a text cell did not benefit from the previous fix. So while the tokens stayed readable when looking at them, they could go back to being unreadable when editing them after double clicking a cell (with automatic light text in dark theme, on a light custom bg, for example).
Proposed solution
Make sure the choices text box also applies the "readable colors combo" calculation.
Related issues
#1155, see this comment: #1155 (comment)
Has this been tested?
Screenshots / Screencasts
Before:
choices-editor-before.mp4
After:
choices-editor-after.mp4
Sorry it took so long @S7venLights 😬