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

feat(application-config): Enforce uniqueness for uriPath of the submenu links. #2567

Merged
merged 6 commits into from
May 3, 2022
Merged

feat(application-config): Enforce uniqueness for uriPath of the submenu links. #2567

merged 6 commits into from
May 3, 2022

Conversation

Rhotimee
Copy link
Contributor

@Rhotimee Rhotimee commented Apr 25, 2022 •
edited
Loading

Summary

SubmenuLinks are meant to be "unique" based on the uriPath
An error is thrown if there are duplicate uriPath in the submenuLinks.

SHIELD-469

@vercel
Copy link

vercel bot commented Apr 25, 2022 •
edited
Loading

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

Name Status Preview Updated
merchant-center-application-kit ✅ Ready (Inspect) Visit Preview May 2, 2022 at 1:21PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Apr 25, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 5b73315

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@commercetools-frontend/application-config Patch
@commercetools-frontend/application-shell Patch
@commercetools-frontend/cypress Patch
@commercetools-frontend/mc-html-template Patch
@commercetools-frontend/mc-scripts Patch
merchant-center-application-template-starter Patch
playground Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

emmenko
emmenko reviewed Apr 25, 2022
View reviewed changes
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Nice idea extending the schema with the ajv-keywords (didn't know about it).

However, I believe it does not fully cover our requirements.

packages/application-config/src/validate-config.ts Outdated Show resolved Hide resolved
@tdeekens
Copy link
Contributor

Maybe worthwhile double checking internally. I think one app alternates between the same sub menu right now using toggles.

@emmenko
Copy link
Member

emmenko commented Apr 25, 2022

I think one app alternates between the same sub menu right now using toggles.

Hmm I double checked. I guess you are referring to product types / attribute groups. From what I understand, the main reason for using the same link with different toggles is to have different labels.

We want to enforce uniqueness so I guess we would need to change the workaround. I'll check with the team responsible for that.

@tdeekens
Copy link
Contributor

different toggles is to have different labels.

Yes. It is temporary. Probably best to have a chat with them.

@Rhotimee Rhotimee requested a review from a team April 26, 2022 09:15
@vercel vercel bot temporarily deployed to Preview April 26, 2022 09:15 Inactive
emmenko
emmenko reviewed Apr 26, 2022
View reviewed changes
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Thanks!

.changeset/bright-hornets-enjoy.md Outdated Show resolved Hide resolved
.changeset/bright-hornets-enjoy.md Outdated Show resolved Hide resolved
packages/application-config/src/validate-config.ts Outdated Show resolved Hide resolved
@Rhotimee Rhotimee marked this pull request as draft April 26, 2022 17:08
@Rhotimee Rhotimee changed the title feat(application-config): Throw error for duplicate uriPath in submenuLink feat(application-config): Enforce uniqueness for uriPath of the submenu links. May 2, 2022
@Rhotimee Rhotimee changed the title feat(application-config): Enforce uniqueness for uriPath of the submenu links. feat(application-config): Enforce uniqueness for uriPath of the submenu links. May 2, 2022
@vercel vercel bot temporarily deployed to Preview May 2, 2022 10:35 Inactive
@Rhotimee Rhotimee marked this pull request as ready for review May 2, 2022 10:35
CarlosCortizasCT
CarlosCortizasCT approved these changes May 2, 2022
View reviewed changes
Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT left a comment

Choose a reason for hiding this comment

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

💯

@vercel vercel bot temporarily deployed to Preview May 2, 2022 10:47 Inactive
@Rhotimee Rhotimee requested a review from a team May 2, 2022 11:00
emmenko
emmenko approved these changes May 2, 2022
View reviewed changes
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now 👌

packages/application-config/src/validations.ts Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview May 2, 2022 13:21 Inactive
@Rhotimee Rhotimee merged commit 3094da2 into main May 3, 2022
@Rhotimee Rhotimee deleted the iy-shield-469-duplicate-submenulink branch May 3, 2022 08:48
@ghost ghost mentioned this pull request May 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmenko emmenko emmenko approved these changes

@ddouglasz ddouglasz ddouglasz approved these changes

@kark kark kark approved these changes

@CarlosCortizasCT CarlosCortizasCT CarlosCortizasCT approved these changes

Assignees

@Rhotimee Rhotimee

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Rhotimee @tdeekens @emmenko @ddouglasz @kark @CarlosCortizasCT