Conversation
Co-authored-by: mkloeppe <mkloeppe@users.noreply.github.com>
… 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.
Have you considered inheriting from both Details and List views?
There was a problem hiding this comment.
Not yet. Could you explain how that might help with our issues?
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
I talked about this with @slint last year and he suggested doing it this way. I understand your concerns but I'm not sure how I could implement this the way you suggest.
5 participants
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: