-
Notifications
You must be signed in to change notification settings - Fork 302
Making it so we can target detail page vs show configuration #16203
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
Conversation
shell/components/Tabbed/index.vue
Outdated
| data() { | ||
| const extensionTabs = this.showExtensionTabs ? getApplicableExtensionEnhancements(this, ExtensionPoint.TAB, TabLocation.RESOURCE_DETAIL, this.$route, this, this.extensionParams) || [] : []; | ||
| const location = this.isResourceDetailDrawer ? TabLocation.RESOURCE_SHOW_CONFIGURATION : TabLocation.RESOURCE_DETAIL; |
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.
@aalves08 @codyrancher just want to confirm my comment in #16189 (comment).
The issue needs updating to reflect that we need it primarily on the edit page and then the show config tab, so two tab locations.
We probably need to be more explicit as well (if a --> y, if b --> x) instead of falling back on a location (if then else). Tabs can be used in a number of different places
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.
@richard-cox we are safe with the create and edit scenarios because of the url param create (i just need to double-check if it works of if I need a PR, but in node we can't do Create's) and the edit has the mode query param, which is working fine.
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.
It may be technically possible to overload the resource detail location + also supply some additional restrictions, but from the perspective of a ui extension developer having distinct tab locations is much easier to understand and use. This also follows the general messages of the three primary resource pages
- List page
- Detail page
- Create/Edit page
| /** | ||
| * Hide the product if the type is present (opposite of ifHaveType) | ||
| */ | ||
| ifNotHaveType?: string | RegExp; |
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.
careful about this... probably is because it's missing a rebase or something. This was added quite recently. Should not be deleted
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.
Fixed. Really not sure what caused it.
|
@codyrancher overall path looks great. I've tried the PR and it works just fine. Thank you very much! If you want I can take care of the documentation part in a separate PR 🙇 |
|
FYI, you can target: EDIT CREATE |
0422aff to
a9765bf
Compare
@aalves08 I cleaned it up and added some docs. If we do want to make separate location I'm happy to do that as well but it's up to you. Edit: do we want to add each of the scenarios to https://github.com/rancher/ui-plugin-examples to make it easier for QA to test? |
a9765bf to
2499dc8
Compare
richard-cox
left a comment
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.
Re-iterating #16203 (comment).
The developer experience is not good, we should have tab locations for each of the three places.
aalves08
left a comment
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.
LGTM @codyrancher
Created new isssue #16218 to be tackled separately @richard-cox |
|
@codyrancher I can take care of the ui-plugins-examples, as per #16219. I've got more stuff to add there |
aalves08
left a comment
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.
minor changes, but let's try to get this in today 🙏 Thanks @codyrancher !
| | Key | Type | Description | | ||
| |---|---|---| | ||
| |`TabLocation.RESOURCE_DETAIL`| String | Location for a Tab on a Resource Detail page | | ||
| |`TabLocation.RESOURCE_SHOW_CONFIGURATION`| String | Location for a Tab on a Resource Show Configuration | |
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.
Ah, the docs content is correct but where you've edited them is wrong... It should be docusaurus/docs/extensions/api/tabs.md not the versioned_docs part as that's old stuff...
Also, minor change but important, can you add a reference that this available from 2.14 and onwards? 🙏
*(From Rancher version v2.14.0)*
Dismissing review because created issue to tackle it separately
2499dc8 to
7506ba8
Compare
aalves08
left a comment
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.
LGTM
Summary
Fixes #16179
Fixes #16189
Occurred changes and/or fixed issues
Made is so we can now differentiate between detail pages and show configuration when adding tabs via extensions.
Technical notes summary
This is done by using provide/inject to indicate to all ancestors that they're inside a resourcedetailpage. It's not the most elegant but it seems preferable over prop drilling especially when we don't control all detail pages that are rendered.
Areas or cases that should be tested
Extension tabs on the resource detail page, show configuration page, create and edit pages.
Areas which could experience regressions
See above
Screenshot/Video
tabular.mp4
Checklist
Admin,Standard UserandUser Base