-
Notifications
You must be signed in to change notification settings - Fork 390
upcoming: [UIE-9813] - Implement routing for Cloud Manager Marketplace #13222
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
base: develop
Are you sure you want to change the base?
upcoming: [UIE-9813] - Implement routing for Cloud Manager Marketplace #13222
Conversation
Cloud Manager UI test results🔺 1 failing test on test run #5 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/clone-linode.spec.ts" |
|||||||||||||||||
| display: 'Marketplace', | ||
| display: !isMarketplaceFeatureEnabled | ||
| ? 'Marketplace' | ||
| : 'Quick Deploy Apps', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly recommend revisiting your naming conventions. You're "enabling marketplace", which essentially is removing marketplace. This is the kind of semantic logic pitfall that leads to bad DX and bugs down the line. Not sure if you had semantic requirements for your tickets, but if so I'd advise to correct the course before moving further. At the very least, if you can't change your flag (which i think you still should do), make sure your logic utilities reference quickDeployApps, not an existing Cloud Manager naming convention. (maybe "cloudMarketplace", marketplaceV2, but anything more explicit then marketplace whcih is already the feature name will go a long way).
It also is the opposite of the account capability you're introducing VS the legacy capability..
|
|
||
| interface Marketplace extends BetaFeatureFlag { | ||
| ga: boolean; | ||
| la: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't la === false the same as ga?
Description 📝
This PR implements the feature flag for the Marketplace feature and implements the routes for it.
Changes 🔄
List any change(s) relevant to the reviewer.
PrimaryNavaccording to the UX mockupsScope 🚢
Upon production release, changes in this PR will be visible to:
Preview 📷
Include a screenshot
<img src="" />or video<video src="" />of the change.🔒 Use the Mask Sensitive Data setting for security.
💡 For changes requiring multiple steps to validate, prefer a video for clarity.
How to test 🧪
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅