fix Set text color of selection #2289
Conversation
There was a problem hiding this comment.
Thanks! Please fix lint errors and tests, and I'll do a full review. It should automatically lint on prepush so I'm not sure how the lint errors got pushed.
We will need to add unit tests to cover the new functionality. An RTL test should allow you to set a browser selection and then set the text color.
If that doesn't work, we can create a Puppeteer test, but RTL is preferred.
|
@raineorshine , now, I'm fixing the lint and test issues. |
|
Hello @raineorshine . I tried to implement Unit test using RTL. |
There was a problem hiding this comment.
Thank you! Below you'll find a complete review.
You did a good job fitting into the existing structure, modifying stripStyleAttribute and using the execCommand infrastructure that was already in place. I didn't know about foreColor and backColor, so that's great that we can implement this with close to the same logic already used for bold, italic, underline, and strikethrough.
We will need to add puppeteer tests since neither jsdom nor happy-dom supports document.execCommand.
It works well, but for one aspect of the behavior: foreground and background colors should not be mixed and matched. Take a look at closer look at the existing functionality in main. Each swatch is a foreground+background combo. If you select blue text, the background color is automatically removed. If you select a blue background, the foreground color is automatically removed. This limits the combinations and ensures proper color contrast. Let me know if that needs any clarification.
Otherwise, they were mostly small suggestions, either technical or stylistic. Watch out for code duplication and avoid mutations if at all possible.
Thanks, and let me know if you have any questions!
raineorshine
force-pushed
the
main
branch
4 times, most recently
from
2231978
to
bbb8780
Compare
|
Thanks for your clear explanation. I will fix this and let you know. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
raineorshine
force-pushed
the
main
branch
3 times, most recently
from
80019f7
to
10b9d7c
Compare
There was a problem hiding this comment.
Thanks! It's coming along well.
The selection should be preserved when the color is set (just like when using bold, italic, underline, and strikethrough). You may need to save and restore the selection in the action-creator.
| (command: 'bold' | 'italic' | 'strikethrough' | 'underline'): Thunk => | ||
| ( | ||
| command: 'bold' | 'italic' | 'strikethrough' | 'underline' | 'foreColor' | 'backColor', | ||
| color: string = '#ffffff', |
There was a problem hiding this comment.
We can't default to #ffffff since it depends on the theme.
| const thought = pathToThought(state, state.cursor) | ||
| const thoughtText = thought.value.replace(/<[^>]*>/g, '') | ||
| // set bullet to text color when the entire thought selected | ||
| if ((selection.text()?.length === 0 && thoughtText.length !== 0) || selection.text()?.length === thoughtText.length) { |
There was a problem hiding this comment.
You need to move the selection logic into the action-creator. Reducers must be pure functions and not rely on any global or window context. state1 + args → state2
| @@ -0,0 +1,35 @@ | |||
| import helpers from '../helpers' | |||
There was a problem hiding this comment.
This looks good.
There are few other tests we need to port over from the old RTL test:
- Clear the text color when selecting white
- Clear background color when selecting text color
- Change color to black when setting background color (this is broken)
|
Note that there are some linting errors. (Is your pre-push hook working?) $ ls .hooks
post-commit* pre-push*
$ cat .hooks/pre-push
npm run lint |
|
@raineorshine |
|
Of course! No problem. |
|
Hello @raineorshine . |
|
Thanks, let me know if any questions come up :). |
|
I got sick during my long trip. |
|
Of course! So sorry to hear you got sick. Hope you get well soon. |
This pull request implements #2267.
Fix the setting color to current selection of thought.