Skip to content
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

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

SarahW91
Copy link
Contributor

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:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

karkraeg and others added 30 commits June 13, 2024 10:07
…nd results in different files and cleaned up code.
@SarahW91 SarahW91 linked an issue Jun 26, 2024 that may be closed by this pull request
@SarahW91
Copy link
Contributor Author

This is based on #310 and that PR should probably be merged first.
I have some questions left that I added as TODO comments in the code - whoever reviews this: would be great if you could help me out with those.

search_sort_config_name = "VOCABULARIES_TYPES_SORT_OPTIONS"


class VocabularyDetailsListView(AdminResourceListView):
Copy link
Contributor

@kpsherva kpsherva Jul 2, 2024

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

@karkraeg karkraeg mentioned this pull request Jul 4, 2024
10 tasks
@SarahW91 SarahW91 marked this pull request as draft July 10, 2024 12:12
@SarahW91 SarahW91 marked this pull request as ready for review July 16, 2024 14:51
self.resource_name if self.resource_name else self.pid_path
),
}
vocab_admin_cfg = current_app.config["VOCABULARIES_ADMINISTRATION_CONFIG"]
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin panel: Add Vocabulary item list view
5 participants