-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
Revert manage theme followup #2889
Revert manage theme followup #2889
Conversation
|
> | ||
Add group | ||
{t('addGroup')} |
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.
The
seems to be unnecessary, and feels like an antipattern to use HTML characters in locale files. Tried resizing the plugin to minimum width and the button stayed on one line.
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.
agree should also be solveable with css
…kens-studio/figma-plugin into fix/2880-revert-manage-theme-followup
…kens-studio/figma-plugin into fix/2880-revert-manage-theme-followup
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 one small comment in slack
> | ||
Add group | ||
{t('addGroup')} |
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.
agree should also be solveable with css
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.
A couple of minor tweaks. Good!
@@ -117,7 +127,8 @@ export const CreateOrEditThemeForm: React.FC<React.PropsWithChildren<React.Props | |||
autofocus | |||
data-testid="create-or-edit-theme-form--group--name" | |||
{...register('group')} | |||
placeholder="Add group" | |||
placeholder={t('addGroup')} | |||
onKeyDown={handleKeyDown('group')} |
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.
Would this not be calling it all the time on any key down event? 🧐
onKeyDown={handleKeyDown('group')} | |
onKeyDown={() => handleKeyDown('group')} |
would that work?
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.
Changed the function handleKeyDown
to handleGroupKeyDown
, the 'group'
argument here is redundant since the function is only being used in one place.
Why does this PR exist?
Resolve comments in #2883
Closes #2900
What does this pull request do?
Testing this change
Theme: <Theme Name>
thenManage themes >
Esc
and confirm that the modal isn't closedCancel
andConfirm
buttons work correctlyCreate new group
Esc
doesn't close the modalClose
button works correctly and isn't blockedAdditional Notes (if any)
After