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

Vulcan: Problem List: Ancestor problem lists #2231

Merged
merged 4 commits into from
Feb 7, 2024

Conversation

ianrohde
Copy link
Contributor

@ianrohde ianrohde commented Feb 6, 2024

Issue

Add ancestor Problems to Problems List page when available. See issue description for further details.

Mock-up text is not final. Figure out final copy with Max.

Text will be updated separately in https://github.com/iFixit/ifixit/issues/51815

CR/QA

/Troubleshooting/dryer (no ancestors)
/Troubleshooting/AEG_T865901H_dryer (has ancestors)

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

Copy link

vercel bot commented Feb 6, 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 6:56pm
react-commerce-prod ✅ Ready (Inspect) Visit Preview Feb 7, 2024 6:56pm

@ghost
Copy link

ghost commented Feb 6, 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 6, 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! 🙌

hcutrone
hcutrone previously approved these changes Feb 7, 2024
Copy link
Contributor

@hcutrone hcutrone left a comment

Choose a reason for hiding this comment

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

CR 🌯 Looks good to me!

@kthaler kthaler self-assigned this Feb 7, 2024
@kthaler kthaler added the QAing Under QA team review label Feb 7, 2024
@kthaler
Copy link
Contributor

kthaler commented Feb 7, 2024

QA 💎 - When a device has problem pages, and it's ancestor also has problem pages, the device's troubleshooting page will display it's own problem pages, and in a second heading it will show it's ancestor's problem pages.

deploy_block 🦖

  1. What is expected when there are a hierarchy of children with problem pages? Should a child-of-a-child device inherit the base problem pages or it's own parent problem pages, or both? For example, I made a child of the AEG T865901H dryer device and gave it a troubleshooting page: https://me.cominor.com/Device/child_test . In the troubleshooting section of the device page, only the dryer troubleshooting pages are inherited, and not the AEG T865901H dryer troubleshooting page. In the Troubleshooting page of the child device, the AEG T865901H dryer Troubleshooting page isn't there, only the dryer pages.

Text will be updated separately in https://github.com/iFixit/ifixit/issues/51815

Not sure if my issue applies then, so please disregard if this isn't in the scope of the pull.
At widths of 767px to 575px, the h2 heading seems too close to the navbar. Video:

Screen.Recording.2024-02-06.at.5.19.43.PM.mov

@kthaler kthaler removed the QAing Under QA team review label Feb 7, 2024
@ianrohde
Copy link
Contributor Author

ianrohde commented Feb 7, 2024

At widths of 767px to 575px, the h2 heading seems too close to the navbar.

This should be corrected, thanks!

Fixed in 4f95bc5

@andyg0808
Copy link
Contributor

What is expected when there are a hierarchy of children with problem pages? Should a child-of-a-child device inherit the base problem pages or it's own parent problem pages, or both?

The troubleshooting pages of a device with the inheritance flag are inherited by all its child devices. No pages from child devices are inherited by more distant descendants.
For full details, here's the spec: https://github.com/iFixit/ifixit/issues/48260

For example, I made a child of the AEG T865901H dryer device and gave it a troubleshooting page: me.cominor.com/Device/child_test. In the troubleshooting section of the device page, only the dryer troubleshooting pages are inherited, and not the AEG T865901H dryer troubleshooting page.

👍 That's what we want. We can add the inheritance flag to the AEG T865901H dryer to have its troubleshooting pages also get inherited, at which point the child_test device should have both Dryer and AEG T865901H troubleshooting pages.

un_deploy_block 👶

hcutrone
hcutrone previously approved these changes Feb 7, 2024
Copy link
Contributor

@hcutrone hcutrone left a comment

Choose a reason for hiding this comment

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

CR 🌯

@deltuh-vee deltuh-vee self-assigned this Feb 7, 2024
@deltuh-vee deltuh-vee added the QAing Under QA team review label Feb 7, 2024
@deltuh-vee
Copy link
Contributor

deltuh-vee commented Feb 7, 2024

QA 🎬
h2 heading is no longer too close to the navbar.
deploy_block ❓
Did you also mean to make this larger?
CleanShot 2024-02-07 at 10 22 03@2x

@ianrohde
Copy link
Contributor Author

ianrohde commented Feb 7, 2024

@deltuh-vee was that observed on the latest preview? I'm not seeing it: https://react-commerce-prod-gi2hw5yqz-ifixit.vercel.app/Troubleshooting/dryer

It is larger. I'll set the fontSize statically so heading changes won't modify

Details Screenshot 2024-02-07 at 10 46 14 AM

@ianrohde
Copy link
Contributor Author

ianrohde commented Feb 7, 2024

Answers heading fontSize fixed in 35dec8c

un_deploy_block 🟢

@deltuh-vee
Copy link
Contributor

QA 🎬
Larger text has been fixed.

@deltuh-vee deltuh-vee removed the QAing Under QA team review label Feb 7, 2024
@@ -17,7 +17,7 @@ export function Answers({
<Heading
as={hasProblems ? 'h4' : 'h3'}
color="gray.800"
fontSize={{ base: '24px', sm: '30px' }}
fontSize={{ base: '20px', mdPlus: '24px' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is mdPlus a custom breakpoint we created? I haven't seen it before

Copy link
Contributor Author

@ianrohde ianrohde Feb 7, 2024

Choose a reason for hiding this comment

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

Yeah, I added it here: https://github.com/iFixit/react-commerce/pull/2143/files#diff-9e3cebe6686b9c7a2c8f51158b777b7f8d7b7bd53fd8a126776adc1e655bf956R7

tl;dr: Chakra uses min-width @media queries, so we need 'md' + 1px

Copy link
Contributor

@hcutrone hcutrone left a comment

Choose a reason for hiding this comment

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

CR 🌯

@ianrohde ianrohde merged commit 4cc92d1 into main Feb 7, 2024
14 checks passed
@ianrohde ianrohde deleted the problem-list--ancestor-problem-lists branch February 7, 2024 19:42
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.

5 participants