Skip to content
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

fix rename/updatemode and input issues #2199

Merged
merged 5 commits into from
Aug 30, 2023
Merged

fix rename/updatemode and input issues #2199

merged 5 commits into from
Aug 30, 2023

Conversation

six7
Copy link
Collaborator

@six7 six7 commented Aug 30, 2023

Fixes #2031
Fixes #2097
Fixes #1799

This set of changes includes improvements to the user experience and state management, as well as bug fixes. The ConfirmDialog and Input components were updated to ensure immediate focus on the input element after rendering. The useTokens module and EditTokenForm component now store the last used update mode when renaming tokens, improving state management and UI. Two bug fixes were also included, related to issues when renaming tokens or token groups.

  • src/app/components/Input.tsx: setTimeout function removed and focus() called directly on htmlInputRef.current to ensure immediate focus on input element after rendering. (F6c8a7b5L3)
  • src/app/components/ConfirmDialog.tsx: Improved UX by focusing on first input field after ConfirmDialog is shown by removing setTimeout function and adding firstInput to useEffect dependencies in ConfirmDialog.tsx. (F7d9e2c8L1)
  • src/app/store/useTokens.tsx: Stores last used update mode when renaming tokens and groups, and UI now defaults to last used update mode instead of current mode. (F5a6b1e4L1)
  • src/app/components/EditTokenForm.tsx: Stores last used update mode when renaming tokens, ensuring it is selected by default instead of current mode. Improves state management and UI. (F2b5c8f1L1)
  • .changeset/curly-ravens-prove.md and .changeset/young-kangaroos-brake.md: Updated @tokens-studio/figma-plugin to fix bug causing issues when renaming tokens or token groups. (Bug fixes)

@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2023

🦋 Changeset detected

Latest commit: 8e84e9b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@tokens-studio/figma-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

PR Analysis

  • 🎯 Main theme: This PR includes bug fixes and improvements to the user experience and state management. It also updates the ConfirmDialog and Input components to ensure immediate focus on the input element after rendering. Additionally, it improves the state management and UI of the useTokens module and EditTokenForm component.
  • 📝 PR summary: This PR fixes issues related to renaming tokens and token groups. It also improves the user experience and state management, and updates the ConfirmDialog and Input components to ensure immediate focus on the input element after rendering.
  • 📌 Type of PR: Bug fix
  • 🧪 Relevant tests added: No
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: - It would be helpful to include more details in the PR description about the bug fixes and improvements made.

  • Consider adding relevant tests to ensure the changes are properly covered.

  • 🤖 Code feedback:

    • relevant file: src/app/components/ConfirmDialog.tsx
      suggestion: Instead of using a setTimeout function to focus on the input element, you can directly call the focus method on the firstInput.current reference. This will ensure immediate focus after rendering. [medium]
      relevant line: firstInput.current.focus();

    • relevant file: src/app/components/EditTokenForm.tsx
      suggestion: It seems that the updateMode variable is no longer used in this component. You can remove the import statement for updateModeSelector and the updateMode variable declaration. [medium]
      relevant line: - const updateMode = useSelector(updateModeSelector);

    • relevant file: src/app/components/Input.tsx
      suggestion: Instead of using a setTimeout function to focus on the input element, you can directly call the focus method on the htmlInputRef.current reference. This will ensure immediate focus after rendering. [medium]
      relevant line: - setTimeout(() => {

    • relevant file: src/app/store/useTokens.tsx
      suggestion: It seems that the updateMode variable is no longer used in this module. You can remove the import statement for updateModeSelector and the updateMode variable declaration. [medium]
      relevant line: - const updateMode = useSelector(updateModeSelector);

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023

Commit SHA:105cdba0c2eded34ed36039c29125a9fa2c4f6cc

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: fix/fix-updatemode 
Status File % Stmts % Branch % Funcs % Lines
🔴 total 68.28 (-0.02) 59.43 (0) 66.43 (-0.04) 68.53 (-0.02)
🔴 src/app/components/ConfirmDialog.tsx 76.19 (-3.81) 66.66 (-4.17) 78.57 (-2.68) 75.67 (-3.81)
🟢 src/app/components/EditTokenForm.tsx 47.75 (0.03) 28.08 (0) 38.63 (0) 48.53 (0.01)
🔴 src/app/components/Input.tsx 77.77 (-1.17) 82.92 (0) 66.66 (-8.34) 77.77 (-1.17)
🟢 src/app/components/TokenTooltip/TokenTooltip.tsx 80 (5) 83.33 (8.33) 100 (0) 80 (5)
🟢 src/app/store/useTokens.tsx 72.99 (0.2) 55 (0) 69.56 (0) 72.8 (0.22)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023

Commit SHA:105cdba0c2eded34ed36039c29125a9fa2c4f6cc
Current PR reduces the test coverage percentage by 100 for some tests

@six7 six7 marked this pull request as ready for review August 30, 2023 07:06
Comment on lines 19 to 21
if (!children || !React.isValidElement(children)) {
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!children || !React.isValidElement(children)) {
return null;
}
if (!children || !React.isValidElement(children) || showEditForm) {
return null;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as further down, that would cause jumps in layout as the button would not be rendered anymore. we just dont want the tooltip to be renderd

if (!children || !React.isValidElement(children)) {
return null;
}

return (
<Tooltip
side="bottom"
label={(
label={showEditForm ? '' : (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
label={showEditForm ? '' : (
label={(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be better to add the condition above and just return null when the editForm is visible ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would cause the children to also not be rendered though (Tooltip wraps the token button)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah got it. then the best way would be something like if (showEditForm) return children

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll include that in a followup 👍

@six7 six7 merged commit 51ebfea into main Aug 30, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants