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

Troubleshooting: Match navtab style to /Parts page #2232

Closed
wants to merge 2 commits into from

Conversation

aburke07
Copy link
Contributor

@aburke07 aburke07 commented Feb 7, 2024

Overview

Reuses the components from the /Parts nav tabs on troubleshooting view pages. Now the styles are consistent across the tabs.

CR

  1. Do we want to extract this into a single component that we pass the links / names / current link into? Then we can use that component in both places.
  2. Do we also need to make a change to this file? I couldn't get this to affect the rendered page when editing it.

QA

  • The navtabs on /Troubleshooting should now match the tabs on /Parts.
  • When there is no /Parts or /Guides for a device, we disable the link.

Closes https://github.com/iFixit/ifixit/issues/51876

1. Instead of having separate styles, lets just
    import the components that we want to match and
    avoid duplicate code.
2. The only difference is that the troubleshooting
    tabs can be disabled, so we need to add some
    special styling and disable them when there is
    no url.
Copy link

vercel bot commented Feb 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
react-commerce ✅ Ready (Inspect) Visit Preview Feb 7, 2024 5:40pm
react-commerce-prod ✅ Ready (Inspect) Visit Preview Feb 7, 2024 5:40pm

@ghost
Copy link

ghost commented Feb 7, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

Copy link
Contributor

github-actions bot commented Feb 7, 2024

📦 Next.js Bundle Analysis for @ifixit/commerce-frontend

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link
Contributor

@jarstelfox jarstelfox left a comment

Choose a reason for hiding this comment

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

CR 🌵 but dev_block 🌵 because I think we might want to do the inverse: https://github.com/iFixit/ifixit/issues/51876#issuecomment-1932566537

@ianrohde
Copy link
Contributor

ianrohde commented Feb 7, 2024

Do we also need to make a change to this file? I couldn't get this to affect the rendered page when editing it.

That's the Problems List component (e.g. /Troubleshooting/dryer ect), and the main trigger for setting up a shared component. There are differences though. Maybe we should punt that to a separate issue to scope and take it on as time permits.

@ianrohde
Copy link
Contributor

ianrohde commented Feb 7, 2024

dev_block 🌵 because I think we might want to do the inverse: https://github.com/iFixit/ifixit/issues/51876#issuecomment-1932566537

I used the pre-existing NavBar as the foundation for the Problems List NavBar. They should be close aside from a few small things. This pretty much confirms the value of a shared component

@jarstelfox
Copy link
Contributor

Found the convo: https://github.com/iFixit/ifixit/discussions/46122#discussioncomment-4898813

@aburke07
Copy link
Contributor Author

aburke07 commented Feb 7, 2024

Ah, bummer. Closing this because the changes are significantly different.

@aburke07 aburke07 closed this Feb 7, 2024
@aburke07 aburke07 deleted the match-parts-navtab-style branch February 7, 2024 19:31
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