-
Notifications
You must be signed in to change notification settings - Fork 253
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
Re-introduce app provided model view override #3721
Re-introduce app provided model view override #3721
Conversation
w8... wot? What is the code in the retrieve to render the plugin's view instead of the core one? I would have thought we accidentally removed that. Two points:
|
Example Plugin has a model view |
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.
😵 OK so I think I understand how this is working now.
What I'd like to see is three examples when starting the app with the example_plugin
is installed:
- A model provided by
example_plugin
that uses what we provide for it's detail view (what should be working today) - A model providing its own view for its own model (I suppose this is working) but isn't technically an override. It just wants to define its own view.
- A model providing an overridden view for another model that is not its own (the thing we have to use
example_plugin_with_view_override
for as @glennmatthews stated above)
For the latter two cases they should have their own discrete dictionary items in the index.js
- Here are a list of my views for my models (maybe something like
views: {"example_plugin:example_models": Component}
) - Here are a list of the views I want to override elsewhere (like the existing
view_overrides
)
development/nautobot_config.py
Outdated
@@ -33,6 +33,7 @@ | |||
|
|||
PLUGINS = [ | |||
"example_plugin", | |||
"example_plugin_with_view_override", |
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.
Shouldn't be enabled by default; again, it will break tests. It should only be enabled ad-hoc for demo purposes.
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.
Ooooh i understand now 😔
…mizuoebideri1-3711-app-provided-model-view
…eveView from example_plugin_with_view_override/ui/index.js/
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.
📖 📓 📚
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.
This file and view need to be renamed from OverridenView
to OverriddenView
for correct spelling of "overridden", or maybe since that kind of reads weird we might just name it OverrideView
?
export { default as PluginFullWidthPageComponent } from "./FullWidthPage.jsx"; | ||
export { default as ExamplePluginRetrieveViewOverride } from "./CustomView" | ||
export { default as PluginFullWidthPageComponent } from "./FullWidthPage"; | ||
export { default as ExamplePluginOverridenModelView } from "./OverridenView"; |
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.
Same rename to fix the typo needs to happen here, too, please.
import { Box } from '@nautobot/nautobot-ui'; | ||
|
||
export function MyFancyCard(props) { | ||
return <Box body>I am a full-fledge component provided by the default example App. The ID of the object I am displaying is: {props.id}</Box>; |
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.
return <Box body>I am a full-fledge component provided by the default example App. The ID of the object I am displaying is: {props.id}</Box>; | |
return <Box body>I am a full-fledged component provided by the Example App. The ID of the object I am displaying is: {props.id}</Box>; |
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.
Doesn't this belong in #3736 instead? Seems like there's a lot of duplicated code between the two PRs, can you clarify?
64d470c
to
1ea2aa6
Compare
1ea2aa6
to
315abcb
Compare
Removed changes in |
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.
Code looks good to me. At some point we need to document this pattern in the app developer guide - is that in scope here?
I think this story only entails the re-introduction of the app overrides. I will open a follow-up issue that tracks this. |
|
view_overrides: { | ||
"example-plugin:other-models": { | ||
"retrieve": "ExampleAppOverrideModelView" | ||
}, | ||
} |
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.
Can you just add one paragraph to our existing docs to this convention? Not perfect but starts getting these conventions documented.
Closes: #3711
What's Changed
The latest commit has the following:
ExampleModel
uses the Nautobot DefaultObjectRetrieveView
.AnotherExampleModel
uses a CustomizedModelView
fromexample_plugin
.Location
model usesOverridenView
provided byexample_plugin_with_view_override
.TODO