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

refactor: enable routing config via provider, remove linkProps from DatasetSelectorModal #1338

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vgeorge
Copy link
Contributor

@vgeorge vgeorge commented Dec 18, 2024

Related Tickets: #1326 #1108

Description of Changes

  • Remove custom link handling in favor of VedaUIProvider's Link
  • Configure VedaUIProvider with react-router's Link component
  • Remove SmartLink from the exploration container
  • Update DatasetSelectorModal to use the new Link component

Notes & Questions About Changes

We discussed in #1108 approaches for making the library agnostic to the routing library. In this comment, I explained why I think we should avoid relying solely on navigation callbacks, as they degrade the accessibility of the library.

In the approach introduced in this PR, we expand the existing configuration provider to also allow passing routing configuration, making the library compatible with react-router, next.js, or any other routing framework.

This PR became quite large but most changes are on invocation calls to the refactored hook. Please let me know if you have any difficulties when reviewing it.

Validation / Testing

The EnvConfigProvider was replaced by VedaUIProvider. The maps and any other functionality depending on the Mapbox token and API endpoints should work as before.

The DatasetSelectorModal only uses Link when empty. To test, replace the line here with datasets={[]}, visit the exploration page, and confirm that the link in the empty state message works correctly.

- Transform environment config into comprehensive Veda UI configuration
- Add framework-agnostic navigation handling
- Extract environment and navigation into separate type-safe interfaces
- Add useVedaUI hook with context validation
- Improve TypeScript types and error handling
- Remove custom link handling in favor of VedaUIProvider's Link
- Configure VedaUIProvider with react-router Link component
- Remove SmartLink from exploration container
- Update DatasetSelectorModal to use new Link component
Copy link

netlify bot commented Dec 18, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit bed9af9
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6762f7c5bf9732000852b22a
😎 Deploy Preview https://deploy-preview-1338--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sandrahoang686
Copy link
Collaborator

sandrahoang686 commented Dec 19, 2024

I explained why I think we should avoid relying solely on navigation callbacks

@vgeorge - Yep, I agree. Callbacks are meant for actions like buttons which is why I didn't include pageheader as part of this work! Since it should retain the link component. But I have some thoughts with this approach, it might be best to meet and talk over a call. (which we are 🙏🏼 )


interface NavigationConfig {
LinkComponent: React.ElementType<Record<string, any>>;
linkPropName: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

(sorry this is not directly related to what this PR changes but since we are on it) - I wonder if we need to make this as an object (linkProps) and make the linkPropName as a part of it since we can't predict what props to expect.

@vgeorge
Copy link
Contributor Author

vgeorge commented Dec 19, 2024

We had a chat today about this (@sandrahoang686, @hanbyul-here, @dzole0311 and I). The output was:

  • @sandrahoang686 pointed out we should be agnostic on the DatasetSelectModal with routing/nav and will propose a new approach to handling empty state for it to this PR
  • The VedaUI provider, while not necessary in the Modal, is still necessary in other parts of the app and would be kept

Please let me if I missed something, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants