chore: add on-event behavior to allowed list for navigation behaviors #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reduced complexity of determining the url
The original implementation for handling `<navigator>` elements nested inside `<nav-route>` elements relied on storing Dom elements in a map by the id of the route which contained them. This had several problems: - A cache had to be created - The items in the cache could become orphaned from their parent doc - Mutations to the doc would not be reflected in the cache - The cache was never cleared - The id or index of the implementation could change This new solution uses a context to wrap every `hv-route` which contains its own document. This allows child routes to retrieve their Dom element and retrieve any nested navigator direction from the doc. Improvements are also made to the initial route by creating all routes into the stack navigator.
… the existing doc (Instawork#618) As the first step in deep link support, this update supports full merging of the existing dom with an incoming deep link document. The structure of the incoming document remains the same as any other \<navigator\> document structure with the following exceptions: - For any \<navigator\> node, the addition of an attribute `merge="true"` will enable merging the navigator children. Omitting the attribute will cause any \<navigator\> to replace an existing one of the same name - Merging will add any child \<nav-route\> nodes which don't exist and will update those which do - All `selected` values from the current document are reset to "false" to allow the incoming document to set a new selected path In the example below, the deep link provides the following changes: 1. The shifts-route receives a sub-navigator 2. The tabs-navigator receives an additional 'settings' tab 3. The root-navigator receives a new 'account-popup' route 4. The initial route is now `root-navigator->tabs-route->tabs-navigator->shifts->shifts-route->past-shifts` Because all incoming data indicated `merge="true"`, all of the changes were additive. Original: ```xml <doc xmlns="https://hyperview.org/hyperview"> <navigator id="root-navigator" type="stack"> <nav-route id="tabs-route"> <navigator id="tabs-navigator" type="tab"> <nav-route id="live-shifts-route" href="/biz-app-hub" selected="true" /> <nav-route id="shifts-route" href="/biz-app-shift-group-list" /> <nav-route id="account-route" href="/biz-app-account" /> </navigator> </nav-route> </navigator> </doc> ``` Deep link: ```xml <doc xmlns="https://hyperview.org/hyperview"> <navigator id="root-navigator" type="stack" merge="true"> <nav-route id="tabs-route"> <navigator id="tabs-navigator" type="tab" merge="true"> <nav-route id="shifts-route" href="" selected="true"> <navigator id="shifts" type="tab" merge="true"> <nav-route id="upcoming-shifts" href="biz-app-shift-group-list" selected="false" /> <nav-route id="past-shifts" href="/biz-app-shift-group-list'" selected="true" /> </navigator> </nav-route> <nav-route id="settings" href="/biz-app-account" selected="false" /> </navigator> </nav-route> <nav-route id="account-popup" href="/biz-app-account" modal="true" /> </navigator> </doc> ``` Merged: ```xml <doc xmlns="https://hyperview.org/hyperview"> <navigator id="root-navigator" type="stack" merge="true"> <nav-route id="tabs-route" selected="false"> <navigator id="tabs-navigator" type="tab" merge="true"> <nav-route id="live-shifts-route" href="/biz_app/gigs/live_shifts" selected="false" /> <nav-route id="shifts-route" href="/biz_app/gigs/groups" selected="true"> <navigator id="shifts" type="tab" merge="true"> <nav-route id="upcoming-shifts" href="/biz_app/gigs/groups" selected="false" /> <nav-route id="past-shifts" href="/biz_app/gigs/groups" selected="true" /> </navigator> </nav-route> <nav-route id="account-route" href="/biz_app/account" selected="false" /> <nav-route id="settings" href="/biz_app/account" selected="false" /> </navigator> </nav-route> <nav-route id="account-popup" href="/biz_app/account" modal="true" /> </navigator> </doc> ```
- Moving the route search into helpers - Moving the local types into types.ts
…ute (Instawork#623) Reflect the user's selections in the document - Use the 'focus' listener to allow each hv-route to set its selected state within the document - De-select any sibling routes to ensure the current is the only selected Note: I did not use 'blur' to have each route deselect itself as that would cause routes in the background to lose their state. In the video, as the user selects between the different tabs, the various \<nav-route\> children of `tabs-navigator` are shown to reflect current selection. When the `company` modal is popped up, the `company` \<nav-route\> shows selected while its sibling `tabs-route` becomes deselected. Note that the `tabs-navigator` children retain their correct selection state. Asana: https://app.asana.com/0/0/1205197138955014/f https://github.com/Instawork/hyperview/assets/127122858/0687558f-c500-419a-bd69-bbd0843ade72
…vigators (Instawork#624) Previous implementation used 'dynamic' and 'modal' routes as the only two children of `stack` navigators. The urls were cached into a context and retrieved by id. This update allows each defined `\<nav-route\>` to be added to its navigator. These known routes can retain their original url and presentation and reduce the complexity of navigating between them. One note about the implementation: the 'initialParams' passed into each route contain their actual url. When we navigate to a route through a behavior, the new params are merged with the existing. The url had to explicitly be deleted from the object as passing a url:undefined would overwrite the initial url value with an 'undefined' value. Asana: https://app.asana.com/0/0/1205197138955012/f
When an <hv-route> is initially defined to load its own data via url, it
will store its <doc> in state and will pass it down to its children.
When a deep link is processed a provides additional information for the
route, the route must abandon its local state.doc and rely on the
element passed in from its parent.
Initial doc:
``` xmldoc
<doc ...>
<navigator id="tabs-navigator" type="tab">
<nav-route id="shifts-route" href="{% url 'biz-app-sub-navigator' %}" selected="true"/>
</navigator>
</doc>
```
The `<hv-route>` representing `shifts-route` will have loaded its own
`<doc/>` and will have stored it in its state.
Deep link:
``` xmldoc
<doc ...>
<navigator id="tabs-navigator" type="tab">
<nav-route id="shifts-route" href="" selected="true">
<navigator id="shifts" type="tab" merge="true">
<nav-route id="upcoming-shifts" href="{% url 'biz-app-shift-group-list' %}" selected="true"/>
<nav-route id="past-shifts" href="{% url 'biz-app-shift-group-list' %}" selected="false"/>
</navigator>
</nav-route>
</navigator>
</doc>
```
The deep link has provided additional context for `shifts-route` by now
providing a sub-navigator `shifts` and the relevant routes.
The `tabs-navigator` remains unchanged; it still has one route
`shifts-route`, which means the `<hv-route>` representing `shifts-route`
is not unmounted. However, that route contains a `<doc>` in state which
is now invalid.
This code change allows the `<hv-route>` to prioritize data received
from its parent over any doc it has in state.
Asana: https://app.asana.com/0/0/1205197143810570/f
Using the `cardStyleInterpolator` option to force iOS to render modals at full screen while retaining their intended animation. | Default | Updated iOS | Updated Android | | ------ | ----- | ----- | |  |  |  | Asana: https://app.asana.com/0/0/1205197143810575/f
Changed approach to use `getDerivedStateFromProps` to drive the state.doc value
- use the more performant `getFirstChildTag` since all these locations know their structure - simplified the usage of the internal component to not require its own props type
…nstawork#640) A few code refactors which will help the next step: 1. Created a helper to determine if a route id is one of the generated 'dynamic/modal' routes 2. Centralized the `screenOptions` for stack navigators to reduce redundant code 3. Created a better `getId` function for getting the id of dynamic and defined routes 4. Moved the creation of dynamic routes into its own function 5. Re-use `buildScreen` when building dynamic routes to avoid redundant code
When a modal is opened, there may be additional navigation which happens within its content. If the modal implements its own stack, those navigation actions can occur within the view of the modal. They can be closed without having to back out of all the screens. In the following videos, the 'company' view is defined as a modal. Without the stack navigator, the 'edit' action replaces the 'company' view and the user has to go back before closing the modal. With the stack navigator, the user can close the modal at any point. Note: default react-navigation UI is being used in these videos to simplify the navigation. | No stack | With stack | | ---- | ---- | |  |  | Asana: https://app.asana.com/0/1204008699308084/1205225573228523/f
Replaced all uses of 'dynamic' as a type of route to 'card' to better reflect the implementation details.
…work#643) Remove instances of `const { someFunction } = this` and `const { someProp } = props`
The initial document may contain multiple <nav-route> elements in a stack. These will be pushed onto the stack immediately. As they are closed they should be removed from the dom to reflect the current state. Note: card and modal routes pushed onto the stack by user activity will not be added to the dom to reflect state. Asana: https://app.asana.com/0/0/1205225573228534/f --- Example: closing the 'company' modal. Before: ```XML <doc ...> <navigator id="root-navigator" type="stack"> <nav-route id="tabs-route" selected="false"> <navigator id="tabs-navigator" type="tab"> <nav-route id="live-shifts-route" href="/biz_app/gigs/live_shifts" selected="false"/> <nav-route id="shifts-route" href="/biz_app/sub-navigator" selected="true"/> <nav-route id="account-route" href="/biz_app/account" selected="false"/> </navigator> </nav-route> <nav-route id="company" href="/biz_app/account/company" modal="true" selected="true"/> </navigator> </doc> ``` After: ```XML <doc ...> <navigator id="root-navigator" type="stack"> <nav-route id="tabs-route" selected="false"> <navigator id="tabs-navigator" type="tab"> <nav-route id="live-shifts-route" href="/biz_app/gigs/live_shifts" selected="false"/> <nav-route id="shifts-route" href="/biz_app/sub-navigator" selected="true"/> <nav-route id="account-route" href="/biz_app/account" selected="false"/> </navigator> </nav-route> </navigator> </doc> ```
No need to pass separate props object into the components
Using constants for consistency
- Merged the required new functionality from `route-doc` into the newly added `DocContext` - Updated the implementation of HvRoute to provide the getter and setter - Cleaned up usage and naming of the value --------- Co-authored-by: Florent Bonomo <[email protected]>
…ork#691) This is a working prototype of deep links with custom navigators. - For stack navigators, the custom navigator provides an injection of a custom router which overrides the behavior of initial state and updates for new routes - In the override, all routes are pushed onto the stack - For tab navigators, the custom navigator calls a 'navigate' whenever the initial route name prop changes This functionality requires mobile branch https://github.com/Instawork/mobile/tree/hardin/deep-link-url-wip **Tabs** With the default behavior, the initial state is correct: shifts-route (middle tab), and second-shifts (right sub tab). However, the deep link has no effect. The current tab selection remains. Deep link ```xml <doc {% namespaces %}> <navigator id="root-navigator" type="stack" merge="true"> <nav-route id="tabs-route"> <navigator id="tabs-navigator" type="tab" merge="true"> <nav-route id="shifts-route" href="{% url 'biz-app-sub-navigator' %}" selected="true"> <navigator id="shifts" type="tab" merge="true"> <nav-route id="second-shifts" href="{% url 'biz-app-shift-group-list' %}" selected="true"/> </navigator> </nav-route> </navigator> </nav-route> </navigator> </doc> ``` | initial state | deep link | | -- | -- | |  |  | With an override navigator, the deep link is able to change the selection in both the main tab and the sub-tab navigators. | initial state | deep link | | -- | -- | |  |  | **Stacks** With the default behavior, the stack navigator pushes only one screen onto the stack: the one with a naming matching the "initialRouteName" property value. Deep links provide no additional functionality. Initial state (index.xml) ```xml <doc {% namespaces %}> <navigator id="root-navigator" type="stack"> <nav-route id="tabs-route"> <navigator id="tabs-navigator" type="tab"> <nav-route id="live-shifts-route" href="{% url 'biz-app-hub' %}"/> <nav-route id="shifts-route" href="{% url 'biz-app-sub-navigator' %}" selected="true"/> <nav-route id="account-route" href="{% url 'biz-app-account' %}" selected="true"/> </navigator> </nav-route> <nav-route id="company" href="{% url 'biz-app-account-company' %}" modal="true"/> </navigator> </doc> ``` Deep link ```xml <doc {% namespaces %}> <navigator id="root-navigator" type="stack" merge="true"> <nav-route id="account" href="{% url 'biz-app-account' %}" modal="true"/> </navigator> </doc> ``` Given this initial state, the desired outcome is to have the "company" modal on top of the tabs. | initial state | deep link | | -- | -- | |  |  | After overriding the navigator and router, the stack is able to push all screens onto the stack in the initial state and is able to push screens onto the stack when a deep link is received. NOTE: There is currently a bug causing the stack initial state to open and immediately close the modal screen. It is caused by the overrides of tabs and stack working against each other. A separate task will be created to look into fixes. | initial state | deep link | | -- | -- | |  |  | Asana: https://app.asana.com/0/1204008699308084/1205129681586820/f
Fix for issue where a tab navigation is auto closing any stack modals. Asana: https://app.asana.com/0/1204008699308084/1205572796301526/f
# Conflicts: # src/components/hv-list/index.tsx # src/components/hv-section-list/index.tsx # src/contexts/index.ts # src/contexts/navigator-map.tsx # src/core/components/hv-navigator/index.tsx # src/core/components/hv-navigator/types.ts # src/core/components/hv-route/index.tsx # src/core/components/hv-route/types.ts # src/services/dom/helpers-legacy.ts # src/services/navigator/helpers.test.ts # src/services/navigator/helpers.ts # src/types-legacy.ts
Resolving CI issues found after merging master with TS migration into integration branch
Moving the behavior trigger code out of `hyper-ref` into a shared location to allow access by other components Asana: https://app.asana.com/0/1204008699308084/1205741965789631/f
…root (Instawork#727) Follow the commits for the individual steps taken - moved the 'once' handling methods into /services/behaviors - updated `BehaviorOptions` to properly reflect the state of the data - created new `RootOnUpdate` and `OnUpdateCallbacks` types - added `onUpdate` to all props which occur from `hv-root` to `hv-screen` - ported the `onUpdate` method and all of its related methods from `hv-screen` to `hv-root` - use the callbacks prop to inject any of the instance-specific callbacks needed for the `onUpdate` - inject the new `onUpdate` into the props Asana: https://app.asana.com/0/1204008699308084/1205741965789634/f --------- Co-authored-by: Florent Bonomo <[email protected]>
Both `hv-screen` and `hv-route` have the potential to have a null `doc` in state. This can happen from a bad load, or from an override document being provided by a parent component. The code used in `onUpdate` and its related methods were not accounting for the possibility of a null `doc'. - Updated `ScreenState` to reflect the values of the state as assigned in `hv-screen` - Updated callback signature to allow `getDoc` to return null - Updated `hv-root` implementation of `onUpdate` and related to account for null doc - Updated `HvGetRoot` to account for null doc - Updated behavior implementations which used `HvGetRoot` to account for null doc Asana: https://app.asana.com/0/0/1205741965789643/f
…ork#729) Implement `hv-route` as a provider of `onUpdate` to allow navigators to process behaviors Only routes which own a document are providers, any children which do not own a document will inherit. Asana: https://app.asana.com/0/1204008699308084/1205741965789637/f --------- Co-authored-by: Florent Bonomo <[email protected]>
…Instawork#730) Implementation of `<behavior>` elements as children of `<navigator>` elements - Added an optional `<behavior>` child in schema - Gather and cache all "load" behaviors per navigator - warn developer of any behaviors with non matching triggers - trigger all behaviors on component load - Refresh and re-trigger behaviors on content update See individual commits for a few clean up steps performed as part of the work Asana: https://app.asana.com/0/1204008699308084/1205741965790735/f
Deep link requests were not properly triggering navigation events like tab selections. Behaviors continued to work as expected. Cause: There was a chunk of code which was calling `useContext` in the hv-navigator for two contexts. The result of these were not being used so [the code was removed](Instawork@f3e25c0). Without that context, the `useEffect` of the custom navigators doesn't fire. Adding it back as part of the hierarchy restores the functionality. In the videos below, the deep link should cause a selection of the "Accounts" tab. | Before | After | | -- | -- | |  |  |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds support for 'on-event' triggered behaviors in navigation, particularly for deep linking. Key changes include:
- Modified schema/core.xsd to allow elements in and added 'merge' attribute to and 'modal' attribute to .
- Implemented HvNavigator component in src/core/components/hv-navigator/index.tsx, supporting stack and tab navigation with nested navigators and dynamic routes.
- Updated src/core/components/hv-root/index.tsx to handle 'DEEP_LINK' and 'DISPATCH_EVENT' actions, with checks for potential event loops.
- Refactored src/core/components/hv-route/index.tsx to support on-event behaviors and improve deep linking functionality.
- Added new navigation helpers in src/services/navigator/helpers.ts for handling dynamic routes and improved route selection and merging.
These changes significantly enhance the flexibility and customization of navigation behaviors in Hyperview, particularly for deep linking scenarios.
30 file(s) reviewed, 26 comment(s)
Edit PR Review Bot Settings
Comment on lines
+42
to
+46
| export type StackScreenOptions = { | ||
| headerMode: 'float' | 'screen' | undefined; | ||
| headerShown: boolean; | ||
| title: string | undefined; | ||
| }; |
There was a problem hiding this comment.
style: Consider using a union type for headerMode instead of including undefined.
Comment on lines
+269
to
+274
| // Check for event loop formation | ||
| if (trigger === 'on-event') { | ||
| throw new Error( | ||
| 'trigger="on-event" and action="dispatch-event" cannot be used on the same element', | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: This check prevents infinite loops, but it might be too restrictive. Consider allowing 'on-event' triggers for dispatch-event actions in certain controlled scenarios
|
|
||
| export type EventMapBase = { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| data?: any; |
There was a problem hiding this comment.
style: Using any type reduces type safety. Consider defining a more specific type
Comment on lines
+45
to
+47
| if (!node || !node.childNodes) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
style: Consider combining these conditions for more concise code:
Suggested change
| if (!node || !node.childNodes) { | |
| return null; | |
| } | |
| if (!node?.childNodes) { |
Comment on lines
60
to
62
| const selectedChild = elements.find( | ||
| child => child.getAttribute('selected')?.toLowerCase() === 'true', | ||
| child => child.getAttribute(Types.KEY_SELECTED) === 'true', | ||
| ); |
There was a problem hiding this comment.
style: Use optional chaining for getAttribute to handle potential null elements
Comment on lines
+296
to
+302
| const cleanedParams: TypesLegacy.NavigationRouteParams = { ...routeParams }; | ||
| if (cleanedParams.url && isUrlFragment(cleanedParams.url)) { | ||
| // When a fragment is used, the original url is used for the route | ||
| // setting url to undefined will overwrite the value, so the url has to be | ||
| // deleted to allow merging the params while retaining the original url | ||
| delete cleanedParams.url; | ||
| } |
There was a problem hiding this comment.
logic: Deleting the url property might have unintended side effects. Consider setting it to undefined instead
| * If an id is found only in the new doc, the node is added to the current | ||
| * the 'merge' attribute on a navigator determines if the children are merged or replaced | ||
| */ | ||
| const mergeNodes = (current: Element, newNodes: NodeListOf<Node>): void => { |
There was a problem hiding this comment.
style: The mergeNodes function is complex and may benefit from being split into smaller, more focused functions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Deep links would benefit from allowing
on-eventtriggered behaviors