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.

Sign up for GitHub

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

Jump to bottom

Ticket #3209. Adds check by include_in_menu attribute before appendin… #3210

Merged
merged 9 commits into from
Jun 14, 2021
Merged

Ticket #3209. Adds check by include_in_menu attribute before appendin… #3210

merged 9 commits into from
Jun 14, 2021

Conversation

aagasi
Copy link
Contributor

@aagasi aagasi commented Jun 2, 2021 •
edited by sirugh
Loading

…g children categories to the returned array.

Description

Adds a check to avoid returning children categories when doesn't apply.

Closes #3209

Breaking Changes

Breaking because it now does not display content if include_in_menu is false.

Verification Steps

  1. Go to the Categories edit form in the admin and make some of them include_in_menu true, and some false.
  2. Go to your pwa and open the mobile devices view.
  3. Make sure you only see the categories that have include_in_menu true.

@aagasi
Ticket magento#3209. Adds check by include_in_menu attribute before a…
e9efb56 
…ppending children categories to the returned array.
@sirugh sirugh added the needs-triage A pull request or issue that needs to be triaged prior to being synced to JIRA label Jun 2, 2021
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jun 2, 2021 •
edited
Loading

Messages
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 43c808a

@aagasi aagasi closed this Jun 2, 2021
@aagasi aagasi reopened this Jun 2, 2021
@aagasi
Copy link
Contributor Author

aagasi commented Jun 2, 2021

@sirugh any idea why the coverage decreased?

@sirugh
Copy link
Contributor

sirugh commented Jun 2, 2021

@sirugh any idea why the coverage decreased?

You can click "Details" and it should tell you :)

@sirugh sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Jun 5, 2021
@sirugh
fixup test falsy case
b1d8974 
Signed-off-by: sirugh <rugh@adobe.com>
sirugh
sirugh reviewed Jun 7, 2021
View reviewed changes
packages/peregrine/lib/talons/CategoryTree/__tests__/useCategoryTree.spec.js
...result.data.category.children,
{
id: categoryId,
include_in_menu: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the default data result all has include_in_menu: 1, we have to augment/add a case that will check the false branch/path.

@sirugh sirugh removed the needs-triage A pull request or issue that needs to be triaged prior to being synced to JIRA label Jun 7, 2021
@aagasi
Copy link
Contributor Author

aagasi commented Jun 11, 2021

@sirugh I believe we need a reviewer now?

@sirugh
Copy link
Contributor

sirugh commented Jun 11, 2021

Correct :) It's in the queue.

supernova-at
supernova-at requested changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@supernova-at supernova-at left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks and works great, thank you!

@tjwiebell
Copy link
Contributor

tjwiebell commented Jun 14, 2021 •
edited
Loading

@aagasi - During QA I observed this behaving a little different than expected, but will need to confirm with the PM if Luma behavior is expected here. In Luma today the children of a not-in-menu node become siblings of that nodes parent, so that setting shouldn't make the node terminating. Will review with @tkacheva and let you know if this needs any adjustments.

Category Structure:
Screen Shot 2021-06-14 at 9 43 53 AM

Luma:
Screen Shot 2021-06-14 at 9 45 36 AM

Venia:
Screen Shot 2021-06-14 at 10 24 40 AM

@tjwiebell
Copy link
Contributor

Current behavior is acceptable, will create followup task for UX to review. QA Approved ✅

@tjwiebell tjwiebell merged commit e186e1c into magento:develop Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sirugh sirugh sirugh left review comments

@supernova-at supernova-at supernova-at approved these changes

Assignees

@sirugh sirugh

@supernova-at supernova-at

Labels
pkg:peregrine Progress: done version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sidebar menu not filtering by include_in_menu attribute.
5 participants
@aagasi @PWAStudioBot @sirugh @tjwiebell @supernova-at