fix(demo): custom bottom bar component #962
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve the introduction and modification of various components related to a bottom tab bar navigation system. New styles for notification badges are added, and existing styles are updated. The tab navigation structure is redefined to enhance modularity, with new XML files created for specific tab implementations. Additionally, the context management for the bottom tab bar is refactored to use a reducer pattern, improving state management. The Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
demo/backend/navigation/navigator/bottom-tabs/tab-bar-notify-tab-2.xml.njk (1)
1-7: Consider adding inline documentationWhile the file is well-structured and aligns with the PR objectives, consider adding some inline comments to improve clarity:
- Explain the purpose of this specific file in the context of the tab bar system.
- Briefly describe how the
selected_tabandnotify_tabvariables are used in the included template.- If there are similar files for other tab states, mention this to provide context on the overall structure.
This documentation would help other developers understand the modular approach you're using for the tab bar system.
demo/backend/_includes/macros/tabbar-bottom/index.xml.njk (1)
6-6: LGTM! Consider making the key value dynamic.The addition of the
keyattribute to the<select-single>element is a good improvement. It aligns with best practices for list rendering in many frameworks and can help with performance and update reconciliation.To enhance flexibility, consider making the
keyvalue dynamic:- <select-single style="tabbar-bottom" name="tabbar-bottom" key="tabbar-bottom"> + <select-single style="tabbar-bottom" name="tabbar-bottom" key="tabbar-bottom-{{ target }}">This change would allow for multiple unique tab bars if needed in the future, while maintaining the current functionality.
demo/backend/navigation/navigator/bottom-tabs/tab-1.xml.njk (1)
13-13: Good addition ofselected_tabvariable.The introduction of the
selected_tabvariable is a good way to track the active tab. For improved clarity, consider adding a comment explaining its purpose.Consider adding a comment like this:
+ {# Set the currently selected tab #} {% set selected_tab = 1 %}demo/backend/_includes/macros/tabbar-bottom-tab/styles.xml.njk (1)
41-52: LGTM: New notification badge style enhances UI feedbackThe
tabbar-bottom-tab-notifystyle introduces a visually appealing notification badge for the tab bar. This small, red circle positioned at the top-right of the tab provides a clear visual cue for users, enhancing the UI's feedback mechanism.While this addition wasn't explicitly mentioned in the PR objectives, it aligns well with the overall goal of improving the bottom bar component.
Consider adding a
minWidthproperty to ensure the badge remains visible even when scaled down on smaller devices:<style id="tabbar-bottom-tab-notify" backgroundColor="#d92b2b" position="absolute" top="0" right="0" height="6" width="6" + minWidth="6" borderWidth="2" borderColor="#d92b2b" borderRadius="6" />demo/src/Components/BottomTabBar.tsx (1)
31-36: LGTM: Improved rendering control, but consider memoizationThe changes in the
useEffecthook effectively address the double rendering issue mentioned in the PR objectives. The new condition preventssetElementPropsfrom being called multiple times for the same element, which is a good optimization.Adding
setElementPropsto the dependency array is correct according to React's rules of hooks. However, this might cause the effect to run more often than necessary ifsetElementPropsis recreated on every render of the parent component.Consider memoizing the
setElementPropsfunction in the parent component or context provider to prevent unnecessary effect runs. You can use theuseCallbackhook for this purpose:const setElementProps = useCallback((navigator, props) => { // existing implementation }, [/* dependencies */]);This would ensure that
setElementPropsonly changes when its dependencies change, potentially reducing unnecessary effect runs in this component.demo/backend/navigation/navigator/bottom-tabs/tab-bar.xml.njk (1)
8-49: Well-implemented tab options with room for improvement.The structure and implementation of individual tab options are consistent and well-organized. The use of conditional rendering for selection states and notifications is appropriate and aligns with the PR objectives.
Consider extracting the tab option structure into a separate template file. This would improve maintainability and make it easier to add or modify tabs in the future. For example:
<!-- tab-option.xml.njk --> <option {% if selected_tab == tab_id %} selected="true" {% endif %} style="tabbar-bottom-tab" value="custom-tab-{{ tab_id }}" > <behavior trigger="select" action="navigate" target="custom-bottom-tabs-tab-{{ tab_id }}" /> <view style="tabbar-bottom-tab-content"> <view style="tabbar-bottom-tab-icon-selected"> {% include tab_icon_selected %} </view> <view style="tabbar-bottom-tab-icon"> {% include tab_icon %} </view> <text style="tabbar-bottom-tab-label">{{ tab_label }}</text> {% if notify_tab == tab_id %} <view style="tabbar-bottom-tab-notify" /> {% endif %} </view> </option>Then, in the main file:
{% include "tab-option.xml.njk" with { tab_id: 1, tab_icon_selected: 'icons/behaviors-selected.svg', tab_icon: 'icons/behaviors.svg', tab_label: 'Tab 1' } %} {% include "tab-option.xml.njk" with { tab_id: 2, tab_icon_selected: 'icons/advanced-selected.svg', tab_icon: 'icons/advanced.svg', tab_label: 'Tab 2' } %}This approach would make it easier to maintain and extend the tab bar in the future.
demo/backend/navigation/navigator/bottom-tabs/tab-2.xml.njk (1)
19-19: Consider removinghref="#"from thereloadbehaviorWhen using
action="reload", specifyinghref="#"may be unnecessary or could lead to unintended behavior. Omitting thehrefattribute can simplify the behavior and ensure it reloads the current view correctly.Apply this diff to remove the unnecessary
hrefattribute:- <behavior action="reload" href="#" /> + <behavior action="reload" />demo/src/Contexts/BottomTabBar.tsx (1)
74-74: Consider renaming parameterpback topropsfor clarityChanging the parameter name from
propstopmay reduce readability. Usingpropsis a common convention in React components and can improve code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- demo/backend/_includes/macros/tabbar-bottom-tab/styles.xml.njk (1 hunks)
- demo/backend/_includes/macros/tabbar-bottom/index.xml.njk (1 hunks)
- demo/backend/_includes/macros/tabbar-bottom/styles.xml.njk (0 hunks)
- demo/backend/navigation/navigator/bottom-tabs/tab-1.xml.njk (1 hunks)
- demo/backend/navigation/navigator/bottom-tabs/tab-2.xml.njk (1 hunks)
- demo/backend/navigation/navigator/bottom-tabs/tab-bar-notify-tab-2.xml.njk (1 hunks)
- demo/backend/navigation/navigator/bottom-tabs/tab-bar.xml.njk (1 hunks)
- demo/src/Components/BottomTabBar.tsx (3 hunks)
- demo/src/Contexts/BottomTabBar.tsx (1 hunks)
- demo/src/Contexts/index.ts (1 hunks)
- demo/src/Core/BottomTabBar/index.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- demo/backend/_includes/macros/tabbar-bottom/styles.xml.njk
🧰 Additional context used
🔇 Additional comments (22)
demo/src/Contexts/index.ts (1)
1-4: Approve the API change to use a hook-based approach.The replacement of
BottomTabBarContextwithuseBottomTabBarContextin the exports is a positive change. It aligns with React's hooks pattern and provides better encapsulation of the context implementation details. This approach is generally considered a best practice for managing and accessing context in functional components.To ensure this change doesn't break existing code, please run the following script to check for any remaining usages of
BottomTabBarContext:If the script returns any results, those locations will need to be updated to use the new
useBottomTabBarContexthook.✅ Verification successful
Verification Successful: No direct usages of
BottomTabBarContextfound.All references to
BottomTabBarContexthave been correctly replaced withuseBottomTabBarContext. The export ofBottomTabBarContextProviderremains appropriate and does not impact the verification.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usages of BottomTabBarContext # Test: Search for direct imports or usages of BottomTabBarContext # Expect: No results, as it should have been replaced with useBottomTabBarContext rg --type typescript --type javascript 'import.*BottomTabBarContext|useContext\(BottomTabBarContext\)'Length of output: 139
Script:
#!/bin/bash # Description: Check for any remaining usages of BottomTabBarContext # Test: Search for direct imports or usages of BottomTabBarContext # Expect: No results, as it should have been replaced with useBottomTabBarContext rg --glob '*.ts' --glob '*.tsx' 'import.*BottomTabBarContext|useContext\(BottomTabBarContext\)'Length of output: 357
demo/backend/navigation/navigator/bottom-tabs/tab-bar-notify-tab-2.xml.njk (4)
1-4: LGTM: Proper use of YAML front matterThe YAML front matter is correctly structured and defines the permalink for this page. This is good practice for static site generators and helps with routing.
5-6: LGTM: Clear variable declarationThe variables
selected_tabandnotify_tabare clearly defined with meaningful names. Setting both to 2 indicates that the second tab is both selected and has a notification, which aligns with the file name.
7-7: LGTM: Modular template inclusionIncluding the
tab-bar.xml.njktemplate promotes code reuse and maintainability. This approach aligns with the PR objective of streamlining the markup for the tab bar.
1-7: Approved: Effective implementation of modular tab bar configurationThis file successfully implements a modular approach to configuring tab bar states, which aligns well with the PR objectives. The structure is clean, easy to understand, and promotes maintainability and reusability.
Key benefits:
- Modular design allows for easy creation of different tab bar configurations.
- Clear separation of concerns between configuration and implementation.
- Supports the goal of demonstrating server updates to the tab bar effectively.
Great job on improving the demo application's structure!
demo/backend/navigation/navigator/bottom-tabs/tab-1.xml.njk (3)
10-12: Simplified structure with improved scrollability.The introduction of a scrollable view with the description macro is a good improvement. It simplifies the structure while maintaining functionality.
10-14: Overall improvements align well with PR objectives.The changes in this file significantly simplify the structure and adopt a more modular approach, which aligns well with the PR objectives of addressing double rendering and streamlining the markup. The separation of the tab bar into a separate file (
tab-bar.xml.njk) should improve maintainability and potentially help prevent re-render loops.However, to ensure full functionality:
- Verify that the tab switching logic is properly implemented in
tab-bar.xml.njk.- Confirm that server-side updates to the tab bar are still possible with this new structure.
To verify the tab switching logic, please review the content of
tab-bar.xml.njk(as requested in the previous verification script) and check for the presence of navigation logic between tabs.
14-14: Good modularization withtab-bar.xml.njkinclusion.The inclusion of a separate
tab-bar.xml.njkfile is a great step towards modularization. This should improve maintainability and reusability of the tab bar component.Let's verify the existence and content of the included file:
✅ Verification successful
Verification Successful:
tab-bar.xml.njkConfirmedThe
tab-bar.xml.njkfile exists and its content aligns with the requirements, ensuring improved modularization and maintainability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of tab-bar.xml.njk # Test 1: Check if the file exists if fd -p "demo/backend/navigation/navigator/bottom-tabs/tab-bar.xml.njk"; then echo "tab-bar.xml.njk file found." else echo "Error: tab-bar.xml.njk file not found." exit 1 fi # Test 2: Display the content of the file echo "Content of tab-bar.xml.njk:" cat $(fd -p "demo/backend/navigation/navigator/bottom-tabs/tab-bar.xml.njk")Length of output: 2166
demo/backend/_includes/macros/tabbar-bottom-tab/styles.xml.njk (2)
36-40: LGTM: New style enhances tab content layoutThe new
tabbar-bottom-tab-contentstyle improves the layout of tab content by centering items vertically and establishing a relative positioning context. This change aligns with the PR's objective of refactoring the bottom bar component and provides a consistent structure for tab content.
36-52: Summary: Positive enhancements to the bottom tab bar stylesThe introduced styles,
tabbar-bottom-tab-contentandtabbar-bottom-tab-notify, significantly improve the bottom tab bar component. They provide better structure for tab content and introduce a notification feature, enhancing both the functionality and visual feedback of the UI. These changes align well with the PR's objective of refactoring and improving the bottom bar component.demo/src/Components/BottomTabBar.tsx (3)
2-2: LGTM: Improved context usage with custom hookThe change from importing
BottomTabBarContexttouseBottomTabBarContextis a good improvement. Using a custom hook for context access encapsulates the context usage, potentially providing additional functionality or error handling. This change aligns with React best practices for managing context.
Line range hint
1-45: Summary: Changes effectively address PR objectivesThe modifications in this file align well with the PR objective of addressing double rendering in the custom bottom bar component. Key improvements include:
- Switching to a custom hook for context access, improving code organization.
- Implementing a mechanism to prevent multiple calls to
setElementPropsfor the same element.- Correctly updating the
useEffectdependencies.These changes should effectively resolve the rendering issues mentioned in the PR description. The code is now more robust and efficient in handling the bottom tab bar logic.
22-22: LGTM: Simplified context usage, but verify if other context values are neededThe change to destructure
setElementPropsfromuseBottomTabBarContextis good. It simplifies the code and makes it clear which part of the context is being used.However, let's verify if other context values were previously used and are still needed:
✅ Verification successful
Verified: Only
setElementPropsis used fromuseBottomTabBarContextThe destructuring of
setElementPropsfromuseBottomTabBarContextis appropriate and does not impact other context values withinBottomTabBar.tsx.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other uses of BottomTabBarContext in the component # Test: Search for other uses of BottomTabBarContext or ctx in this file rg --type typescript 'BottomTabBarContext|ctx' demo/src/Components/BottomTabBar.tsxLength of output: 120
Script:
#!/bin/bash # Description: Check for any other uses of BottomTabBarContext or ctx in BottomTabBar.tsx # Test: Search for other uses of BottomTabBarContext or ctx in the specified file rg --type ts --max-columns=200 'BottomTabBarContext|ctx' demo/src/Components/BottomTabBar.tsxLength of output: 274
demo/backend/navigation/navigator/bottom-tabs/tab-bar.xml.njk (2)
1-6: LGTM: Component declaration and namespaces are well-structured.The component declaration, namespace usage, and attributes are correctly implemented. This provides a solid foundation for the custom bottom tab bar component.
7-50: Well-structured tab bar implementation aligns with PR objectives.The use of
<select-single>for the tab bar structure is appropriate and supports mutually exclusive tab selection. This implementation aligns well with the PR objective of addressing double rendering issues while still allowing for server-side updates of the tab bar.demo/backend/navigation/navigator/bottom-tabs/tab-2.xml.njk (3)
7-7: Importing the 'button' macro is appropriateThe
buttonmacro is correctly imported to support the button components used below.
18-18: Confirm the usage of thehideattribute inattributesEnsure that
hide:"true"in theattributesdictionary correctly hides the "Remove notification" button initially. Verify thathideis the correct attribute according to the framework's conventions; it might need to behiddenor another attribute depending on the implementation.
22-23: Settingselected_tabcorrectly for tab bar inclusionThe
selected_tabvariable is appropriately set to2, ensuring that the tab bar highlights the current tab. Includingtab-bar.xml.njkintegrates the tab bar component as expected.demo/src/Core/BottomTabBar/index.tsx (3)
18-19: Safe use of optional chaining withgetElementPropsandonUpdateGood use of optional chaining to safely access
getElementPropsandonUpdate, preventing potential runtime errors when these properties are undefined.
31-31: Dependencies inuseCallbackare correctly specifiedThe dependency array
[id, setElement, onUpdate]in theuseCallbackhook is appropriately set, ensuring that the callback updates when these dependencies change.
37-42: Proper use ofRender.renderChildrenThe
Render.renderChildrenfunction is correctly utilized to render the children elements with the updatedonUpdateCustomcallback and other necessary props.demo/src/Contexts/BottomTabBar.tsx (1)
44-72: Refactoring to use reducer pattern enhances state managementGreat job refactoring the context to use the
useReducerhook. This improves state management by making updates more predictable and scalable, especially as the state logic grows in complexity.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- demo/src/Core/BottomTabBar/index.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
demo/src/Core/BottomTabBar/index.tsx (2)
14-19: Good use of context and optional chainingThe code correctly retrieves
getElementPropsandsetElementfrom the context and safely accessespropsusing optional chaining. This enhances robustness and prevents potential errors ifgetElementPropsis undefined.
21-32: Correct use ofuseCallbackwith appropriate dependenciesThe
onUpdateCustomfunction is properly memoized usinguseCallback, and the dependency array includes all necessary dependencies (id,setElement,onUpdate). This ensures that the callback updates correctly when any of these values change, preventing unnecessary re-renders.
Remove most excluded path from schema validation, except for the share behaviors which should likely be moved to a custom behavior.
Although deprecated, bring back in sync with text-input
allow arbitrary strings - since IDs of navigators are not in the same doc.
These files are only meant to be included and should not generate static files (so they are not served nor XML validated). Ultimately we will want to segregate private and public files
flochtililoch
force-pushed
the
florent/bottom-tabs
branch
from
e7c11ec
to
cd8f1ee
Compare
Fix double rendering of the custom bottom bar component by refactoring the following pieces:
getElementProps,setElementProps,setElement.registeredattribute on the element upon setting the props in the context, to avoid re-render loops when the screen holding the custom element re-renders. This does not prevent server side updates of the tab bar.Special thanks to @ehenighan for the thorough investigation and help solving #956.
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-14.at.10.12.35.mp4
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation