Skip to content
This repository has been archived by the owner on Dec 19, 2024. It is now read-only.

Match Device Navigation Tab styling #2234

Merged
merged 6 commits into from
Feb 8, 2024
Merged

Match Device Navigation Tab styling #2234

merged 6 commits into from
Feb 8, 2024

Conversation

aburke07
Copy link
Contributor

@aburke07 aburke07 commented Feb 7, 2024

Overview

Extracts and reuses a styled nav tab component on /Parts, /Troubleshooting/device, and /Troubleshooting pages. Reuses the components from the Troubleshooting nav tabs on parts and problem list view pages. Now the styles are consistent across the tabs.

QA

  • The navtabs on /Parts should now match the tabs on /Troubleshooting pages and problem list pages.
    • All three should now match the updated figma.
  • When there is no /Parts or /Guides for a device, we disable the link.
  • Please check this across all browser widths.

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

1. This is currently duplicated in the Troubleshooting problem list
    pages. They are the exact same styles except for the slightly grey
    background on disabled tabs. We should consolidate this style anyways.
1. Now we just need to pass in the data in the
    correct order and it will be rendered correctly!
1. This has slightly different styling that we
    should still pass in (for now).
2. We also need to set the font size in the navtabs
    so that it is consistent across all the uses.
1. Replaced the right border with larger padding so the breadcrumbs and
    the nav tabs are not so close to each other.
Copy link

sentry-io bot commented Feb 7, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: frontend/templates/product-list/SecondaryNavigation.tsx

Function Unhandled Issue
SecondaryNavigation ReferenceError: padding is not defined SecondaryN...
Event Count: 1 Affected Users: 1

Did you find this useful? React with a 👍 or 👎

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 8, 2024 7:23pm
react-commerce-prod ✅ Ready (Inspect) Visit Preview Feb 8, 2024 7:23pm

@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! 🙌

1. We actually do want this border on all screen sizes except for xl.
2. We should still have the padding on the breadcrumbs, but we now want
    it to be 16px on all screen sizes.
@jordycosta
Copy link
Member

jordycosta commented Feb 8, 2024

dev_block 🟥

On mobile, there seems to be a small box/divider to the left of the nav bar

mobile

1. We don't have the breadcrumbs to the right of these tabs on mobile, so
    the right border is unnecessary and looked weird. This now matches
    the figma.
@aburke07
Copy link
Contributor Author

aburke07 commented Feb 8, 2024

Un_dev_block 👽 Whoops, good catch. Fixed in f692b5f

@erinemay
Copy link
Contributor

erinemay commented Feb 8, 2024

QA 🦖

  • Match Device Navigation Tab styling #2234 (comment) is fixed.
  • The navigation styling is now similar across /Parts, Troubleshooting, and Problem List pages. Padding, font bolding now matches /Troubleshooting.
  • Disabled states are consistent. I couldn't find a /Parts page that doesn't have a corresponding device page. On /Troubleshooting, if there are no parts, the button is disabled.
  • Checked on desktop, tablet, and mobile, Safari and Chrome.

@erinemay erinemay removed the QAing Under QA team review label Feb 8, 2024
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 🌵

@aburke07 aburke07 merged commit 4e3c8e4 into main Feb 8, 2024
14 checks passed
@aburke07 aburke07 deleted the match-navtab-styling branch February 8, 2024 22:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants