Skip to content

Discussion: react-tabs vs. react-router links #2621

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

Closed
lindapaiste opened this issue Nov 19, 2023 · 2 comments · Fixed by #2622
Closed

Discussion: react-tabs vs. react-router links #2621

lindapaiste opened this issue Nov 19, 2023 · 2 comments · Fixed by #2622
Labels
Area: Code Quality For refactoring, cleanup, or improvements to maintainability Enhancement Improvement to an existing feature

Comments

@lindapaiste
Copy link
Collaborator

Increasing Access

Using standard HTML <a> tags for links is more semantically correct and allows for standard right-click behavior.

Feature enhancement details

There are multiple places in the app where we use tabs to switch between content. Some of these are coded using the react-tabs package, while others use <Link> elements from react-router-dom.

It is my opinion that we should use the react-router-dom approach in situations where the tabs have different URLs but I want to open this up for discussion.

Advantages of react-router-dom

  • We can use the router to control the content on the page, instead of manually switching based on pathname
    useEffect(() => {
    if (location.pathname === '/privacy-policy') {
    setSelectedIndex(0);
    } else if (location.pathname === '/terms-of-use') {
    setSelectedIndex(1);
    } else {
    setSelectedIndex(2);
    }
    }, [location]);
  • We don't need any callback function when changing tabs.
    function onSelect(index, lastIndex, event) {
    if (index === lastIndex) return;
    if (index === 0) {
    setSelectedIndex(0);
    history.push('/privacy-policy');
    } else if (index === 1) {
    setSelectedIndex(1);
    history.push('/terms-of-use');
    } else if (index === 2) {
    setSelectedIndex(2);
    history.push('/code-of-conduct');
    }
    }
  • We can automatically highlight the current tab using the NavLink component (instead of Link) https://reactrouter.com/en/main/components/nav-link
  • Links render as HTML <a> elements which is more semantically correct and allows for standard right-click behavior.
    image
  • Screen readers understand links, and they can be focused using Tab navigation.

Current Usage

image
Legal page - uses react-tabs
https://github.com/processing/p5.js-web-editor/blob/185a0b1ed3cef02957bd9bc04d25b33c33eee11d/client/modules/Legal/pages/Legal.jsx

image
Dashboard page - uses react-router-dom (with manual routing and highlighting that I would like to clean up)
https://github.com/processing/p5.js-web-editor/blob/185a0b1ed3cef02957bd9bc04d25b33c33eee11d/client/modules/User/components/DashboardTabSwitcher.jsx

image
Preferences modal - uses react-tabs
This would stay as-is because there is no URL change involved
https://github.com/processing/p5.js-web-editor/blob/185a0b1ed3cef02957bd9bc04d25b33c33eee11d/client/modules/IDE/components/Preferences/index.jsx

@lindapaiste lindapaiste added Enhancement Improvement to an existing feature Area: Code Quality For refactoring, cleanup, or improvements to maintainability labels Nov 19, 2023
@udittripathi
Copy link

udittripathi commented Nov 20, 2023

I can host the application locally using docker but when I make changes in the code it is not getting refreshed in the browser. So I have to build a new image every time or there is something I am doing wrong? Kindly help me with this.

@avengsum
Copy link

@lindapaiste is this issue is solved or anything left that I can work on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Code Quality For refactoring, cleanup, or improvements to maintainability Enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants