-
Notifications
You must be signed in to change notification settings - Fork 109
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
implement lettercase feature #2461
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you! This is a great stat. Thanks for adding a unit test as well.
Please always reference the associated issue number in the PR description. That way GitHub will link the PR to the issue. I've added it above.
It looks like the Linter, Tests, and Puppeteer tests are all failing in the CI. Once these are all passing I can do a proper review.
I will note that useWindowOverlay
has been duplicated in the ColorPicker and LetterCasePicker component. As mentioned in the issue, and following the DRY principle, we want to avoid code duplication. Code duplication doubles the amount of code to maintain and leads to subtle discrepancies over time. Instead, factor out the common code and use the appropriate level of abstraction to share common functionality.
89046c7
to
7c5e319
Compare
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.
Thank you! Looks very good. I've offered some tips below to clean up the code a bit.
There are just a couple behavioral issues I am seeing:
- Let's prevent both the ColorPicker dropdown and the LetterCase dropdown from being open at the same time. If one is clicked, the other one should close.
- The browser selection (i.e. text cursor) is not being preserved when it starts at the end of the thought. I'm guessing that
selection.restore
does not work in this case becausesavedSelection.node
is not the same after the thought re-renders.
src/components/LetterCasePicker.tsx
Outdated
<div style={{ userSelect: 'none' }}> | ||
<div | ||
ref={ref} | ||
style={{ |
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.
This project uses PandaCSS for styling. The style
attribute should only be used for styles with dynamic runtime values. Otherwise you should use Panda's css
function. See other components for usage, or check out https://panda-css.com/docs/concepts/writing-styles.
When you rebase on |
9555022
to
ba8a0b6
Compare
#2424
I implemented lettercase features( LowerCase, UpperCase, SentenceCase, TitleCase) to the ToolBar.
These are the screenshots.