feat(components): Add button Navigation Provider #2270

MichaelParadis · 2024-12-19T14:46:42Z

Motivations

As Atlantis evolves we want to remove the react-router-dom dependency directly used in Atlantis to support other client side routing libraries such as tanstack router. Since the to prop has been deprecated Button the logic was added to ButtonAnchorProps and the client side navigation logic is tied to a new prop called useClientSideRouting

The reason for adding this prop is because a lot of client side routing libraries don't handle urls that navigate to a route not found. For example using react-router-dom to navigate to https://google.com would not navigate to google.com and will instead try and find the path in the router and navigate to the 404 route for that specific router.

Decisions made:

the client side navigation logic is tied to a new prop called useClientSideRouting
- The reason for adding this prop is because a lot of client side routing libraries don't handle urls that navigate to a route not found. For example using react-router-dom to navigate to <baseURL>/https://google.com would not navigate to google.com and will instead try and find the path in the router and navigate to the 404 route for that specific router.
For typescript support developers need to declare the NavigationConfig interface. The reason for this is to provide typescript support when consuming using the provider because regular links in this libraries would have the typehint support.
However due to the age of react-router-dom version 5 the pathnames are not fully typed.
There is also some limitations with tanstack router where the parameter type hints aren't fully enforced

Changes

Added

components: ButtonNavigationProvider to provide support for other client side routing libraries.

Changed

Deprecated

Removed

Fixed

Security

Testing

These changes can be tested by reverting 75acaf51c124371a7c237f4fd4076dcb5ac91dcc

Verify that the links navigate correctly. The internal link will navigate to /components with url query parameters as well.
See draft PRs that mention this PR

Changes can be
tested via Pre-release

In Atlantis we use Github's built in pull request reviews.

cloudflare-workers-and-pages · 2024-12-19T14:48:03Z

Deploying atlantis with Cloudflare Pages

Latest commit:	`d4ffe50`
Status:	✅ Deploy successful!
Preview URL:	https://f0b245f1.atlantis.pages.dev
Branch Preview URL:	https://job-111716-add-button-naviga.atlantis.pages.dev

View logs

…uttons

Move Button types to separate file.

Update documentation to match the renaming

This reverts commit a49bf1e.

github-actions · 2024-12-19T15:51:42Z

Published Pre-release for d4ffe50 with versions:

  - @jobber/[email protected]+d4ffe50a

To install the new version(s) for Web run:

npm install @jobber/[email protected]+d4ffe50a

…sistently when useClientSideRouting isn't set

scotttjob

I've only read through the code so far, but I wanted to drop my comments on my code read as soon as possible (which is why this only a comment).

I'd like to get a chance to run the code and try it out first hand before approving, but it's okay if someone else gets here first.

What's in here looks great, I really appreciate your approach to backwards compatibility and providing paths forward for both of our router types.

Hopefully I'll get a chance to run the code today.

scotttjob · 2025-01-06T13:14:04Z

docs/components/Button/Button.stories.mdx

+      to="/some-path/$someParam"
+      routerOptions={{
+        resetScroll: true,
+        params: { someParam: "someValue" },


How are params/search/state different? They're all just URL Query Params in the end right? Or am I missing something?

Search is URL query params tanstack docs
Params is path params in the url. Similar to how on github has the URL for this PR: https://github.com/GetJobber/atlantis/pull/2270/files which could be formatted as such:
https://github.com/<repoOwner>/<repoName>/pull/<pullRequestID>/files

State is the history state handed by the router. It is from the ToOptions from tanstack router. It is just an arbitrary key value store

scotttjob · 2025-01-06T13:17:23Z

packages/components/src/Button/ButtonNavigationProvider.tsx

+export type RouterOptions = NavigationConfig extends { routerOptions: infer T }
+  ? T
+  : // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    any;


👀 I have a feeling this 'any' is actually an 'any' but if that's the case can we leave that comment at the end of the eslint disable line explaining that? Just so nobody comes back in the future and spends an afternoon trying to type it 'properly'

That is a good call. It is actually an any here so I can update that

scotttjob · 2025-01-06T13:19:50Z

packages/components/src/Button/useLinkButtonProps.ts

+    (event: React.MouseEvent<HTMLAnchorElement | HTMLButtonElement>) => {
+      useClientSideRouting && event.preventDefault();
+
+      onClick?.(event);


I like that we call 'onClick' even when handling the url. I think we had a restriction in place before that said you can't provide an onClick if you also provide a URL? Was I mistaken, or did we remove that restriction (happy if we removed it, I never liked it).

I am 99% sure we removed that restriction a while ago (looking at the before part of the diff the buttons that were diffs didn't exclude the onClick from the base interface

scotttjob · 2025-01-06T13:20:20Z

packages/site/src/content/Button/ButtonNotes.mdx

+}
+```
+
+#### Tanstack router example:


Love that you supported both routers in your examples. Great work!

scotttjob

I dropped you a question in a DM, but otherwise this looks great!

As a follow-up, I wonder if it's worth documenting our approach to the Interface/Provider pattern here, so it can inform the work when we approach soon in the future (when we do the same work with Apollo and any other libraries we would rather depend on, versus consume directly)

scotttjob · 2025-01-08T14:41:16Z

packages/components/src/Button/ButtonNavigationProvider.tsx

+  ) => void;
+
+  /**
+   * Function that will build the href used by the anchor tag in the Button component based on the url prop of the Button


This description + the function name make the functionality of this a bit vague. Maybe just an example of the expected input -> output? Definitely a follow-up at this point.

MichaelParadis added 4 commits December 19, 2024 06:56

Add ButtonNavigationProvider for handling Client side navigation on b…

da0121f

…uttons

Add useLinkButtonProps for event handlers on Buttons that are links

e920aad

Update Button to use new logic for handling links.

f3c8f0e

Move Button types to separate file.

Update meta.json export

39081b9

MichaelParadis force-pushed the JOB-111716/add-button-navigation-provider branch 3 times, most recently from f8b260a to 1d1ce50 Compare December 19, 2024 15:09

MichaelParadis added 2 commits December 19, 2024 07:10

Update Button documentation

6354c42

Update prop comments and nanes

7a91820

Update documentation to match the renaming

MichaelParadis force-pushed the JOB-111716/add-button-navigation-provider branch from 1d1ce50 to 7a91820 Compare December 19, 2024 15:15

MichaelParadis added 3 commits December 19, 2024 07:43

Fix issue with useCallback in useLinkButtonProps

04f1d0e

Commit for testing in new documentation site

a49bf1e

Revert "Commit for testing in new documentation site"

75acaf5

This reverts commit a49bf1e.

MichaelParadis added 2 commits December 19, 2024 10:04

Fix typescript issues relating using the type of urls not working con…

d5574f0

…sistently when useClientSideRouting isn't set

Fixing a type issue when using react-router-dom

d4ffe50

MichaelParadis marked this pull request as ready for review December 19, 2024 21:32

MichaelParadis requested a review from a team as a code owner December 19, 2024 21:32

jobbiebot bot added review requested Team Atlantis labels Dec 19, 2024

scotttjob reviewed Jan 6, 2025

View reviewed changes

scotttjob approved these changes Jan 8, 2025

View reviewed changes

jobbiebot bot added approved and removed review requested labels Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(components): Add button Navigation Provider #2270

feat(components): Add button Navigation Provider #2270

MichaelParadis commented Dec 19, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

scotttjob left a comment

scotttjob Jan 6, 2025

MichaelParadis Jan 7, 2025

scotttjob Jan 6, 2025

MichaelParadis Jan 7, 2025

scotttjob Jan 6, 2025

MichaelParadis Jan 7, 2025

scotttjob Jan 6, 2025

scotttjob left a comment

scotttjob Jan 8, 2025

feat(components): Add button Navigation Provider #2270

Are you sure you want to change the base?

feat(components): Add button Navigation Provider #2270

Conversation

MichaelParadis commented Dec 19, 2024 • edited Loading

Motivations

Decisions made:

Changes

Added

Changed

Deprecated

Removed

Fixed

Security

Testing

cloudflare-workers-and-pages bot commented Dec 19, 2024 • edited Loading

Deploying atlantis with Cloudflare Pages

github-actions bot commented Dec 19, 2024 • edited Loading

scotttjob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotttjob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelParadis commented Dec 19, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading