-
Notifications
You must be signed in to change notification settings - Fork 2
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(ui): Other languages for actions, border fix, nav arrows remov #731
Conversation
WalkthroughThe pull request introduces language-specific functionality to the action fetching mechanism across multiple components. By modifying the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ui/src/lib/fetch/transformers/useApi.ts (1)
5-5
: Ensure a safe default for language parameter
While passing a language parameter is useful, consider providing a fallback (e.g., a default locale) when none is provided. This can prevent potential errors where the language is undefined.-export const useAction = (lang: string) => { +export const useAction = (lang: string = "en") => {ui/src/lib/fetch/transformers/useFetch.ts (1)
77-106
: Consider the trade-offs of infinite staleTime
Currently, the data is never invalidated (staleTime: Infinity). If the underlying data might change, you may want to tailor the staleTime or implement revalidation strategies to ensure data freshness, particularly for dynamic user-facing content.ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx (1)
20-20
: Unused commented-out constant
The commented-out "actionButtonClasses" might be removed entirely if not intended for future use. This helps maintain clean code.-// const actionButtonClasses = "border h-[25px]"; +// Remove this line if no longer neededui/src/features/Editor/components/LeftPanel/components/Actions/index.tsx (2)
Line range hint
199-214
: Conditionally rendering a separator
This check (index !== actions.length - 1) to show a border for all but the last item is straightforward. However, consider removing the bottom border altogether if the design doesn't require it. Also, verify that an empty array of actions doesn't break the logic.
243-246
: Verify empty or single-item edge cases
When rendering the separator for grouped actions, check how the UI behaves if a group has exactly one item. The condition index !== actions[key].length - 1 may be valid, but any off-by-one mistakes in more complex data structures could appear.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ui/src/features/Editor/components/LeftPanel/components/Actions/index.tsx
(6 hunks)ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
(2 hunks)ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx
(5 hunks)ui/src/lib/fetch/transformers/useApi.ts
(1 hunks)ui/src/lib/fetch/transformers/useFetch.ts
(1 hunks)
🔇 Additional comments (9)
ui/src/lib/fetch/transformers/useApi.ts (1)
13-13
: Looks good
The usage of these data fetch hooks is clear, well-structured, and consistent with the new language parameter.
Also applies to: 21-21, 29-29
ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx (3)
5-5
: Imports are consistent
Import statements for i18n and the new components look good. Avoiding unused imports helps maintain clarity.
Also applies to: 9-9
30-30
: Language-specific fetching
Using “useAction(i18n.language)” ensures the parameter schema is localized. This is consistent with the broader approach in this PR to support multiple languages.
Line range hint 47-58
: Removal of navigation arrows
Commenting out the arrow icons aligns with the PR description of removing navigation arrows. Confirm you thoroughly tested any user flows that may have relied on these arrows.
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (2)
10-10
: New i18n import
Importing i18n ensures the dialog can correctly fetch localized actions. Straightforward addition.
33-33
: Leverage language-specific fetching
Calling “useGetActionsSegregated(i18n.language)” properly localizes the list of actions. This is consistent with the rest of the PR changes.
ui/src/features/Editor/components/LeftPanel/components/Actions/index.tsx (3)
21-21
: Check for i18n fallback or defaults
By importing i18n directly here, it’s assumed that the language has been set before mounting this component. If there's any chance the i18n instance hasn’t finished loading or a language isn’t set, you might want to handle a fallback scenario.
36-36
: Pass language dependency carefully
You're now passing i18n.language into the useAction hook. Ensure that any re-renders due to changes in i18n.language behave as expected, especially if the user switches languages on-the-fly. A re-fetch might be desired to update the actions in the new language.
226-226
: Nested map rendering
In your accordion logic, each category is displayed in a nested map. This approach is valid, but ensure that the data shape is always consistent (e.g., actions[key] is never undefined if it’s expected to exist).
@@ -32,7 +33,7 @@ type Props = { | |||
|
|||
const ActionsList: React.FC<Props> = ({ nodes, onNodesChange }) => { | |||
const t = useT(); | |||
const { useGetActions, useGetActionsSegregated } = useAction(); | |||
const { useGetActions, useGetActionsSegregated } = useAction(i18n.language); |
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.
Can you change the language while inside the editor and open the actions panel to see if i18n.language updates correctly? You might need to fetch me and check me's language
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.
Language updates correctly when changed with AccountUpdateDialog. The issue I could see happening is that if a user doesnt have me.language (which is possible for current accounts due to when an account is created, but should not be an issue for any future accounts) it is possibly null and then it falls back on i18n.language (this was in PR 671)
Overview
What I've done
What I haven't done
How I tested
manually
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Bug Fixes
Chores