-
Notifications
You must be signed in to change notification settings - Fork 42
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
Vocabulary item view #347
base: master
Are you sure you want to change the base?
Vocabulary item view #347
Conversation
Co-authored-by: mkloeppe <[email protected]>
… working properly.
…nd results in different files and cleaned up code.
…nt vocabulary types
This is based on #310 and that PR should probably be merged first. |
search_sort_config_name = "VOCABULARIES_TYPES_SORT_OPTIONS" | ||
|
||
|
||
class VocabularyDetailsListView(AdminResourceListView): |
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.
Have you considered inheriting from both Details and List views?
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.
Not yet. Could you explain how that might help with our issues?
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 would ensure less ambiguity on where the methods are actually coming from, this class now only imitates the interface of AdminResourceDetails view in order to use some of its frontend functionalities, while it does not actually inherit from the class itself. It might create divergence on the implementation of these "virtually" inherited methods the code it will be harder to maintain.
So to answer your question, it does not solve immediate problems but prevents ambiguity in the future
…instead of get_context method
self.resource_name if self.resource_name else self.pid_path | ||
), | ||
} | ||
vocab_admin_cfg = current_app.config["VOCABULARIES_ADMINISTRATION_CONFIG"] |
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 like to suggest using class factory pattern here instead of moving it to a static dict config in the configuration variable. I understand the dict is imitating the AdminView class interface but if it is not really a class, then we diverge of the pattern of implementation of the administration module
Description
Addresses #346
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: