-
Notifications
You must be signed in to change notification settings - Fork 81
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
Resolve #295 and #407: Improve the UI/UX in adding/uploading resources #516
Conversation
This looks good but ideally everywhere will just have the one "add" button with the two tabs from there. This approach is to show the user available resources before adding new ones. I'm not sure why they became broken out. |
I see. OK, I'll try switching to the 1-button style with tabbed dialog boxes. |
@@ -45,6 +45,13 @@ class PartiesDetail(LoginPermissionRequiredMixin, | |||
permission_denied_message = error_messages.PARTY_VIEW | |||
attributes_field = 'attributes' | |||
|
|||
def get_context_data(self, *args, **kwargs): |
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 snippet is added to five different classes, and we will likely need something similar for location-to-location and party-to-party relationships at some point. How about creating a mixin for this?
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 did think about creating a mixin, but these methods all currently access the project object in different ways. I think there's a get-project-object mixin for that and I'll try make all of these consistent and then build the resources mixin on top of that.
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 think, all of these classes inherit ProjectMixin
, so self.get_project()
should be available everywhere.
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.
That should work perfectly.
Question: Do I subclass the project-has-resources mixin from ProjectMixin
and inherit from that, or do I just do multiple inheritance from the ProjectMixin
and the project-has-resources mixin?
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.
You can do it either way. If you subclass your project-has-resources mixin from ProjectMixin
, then you make sure self.get_project()
is always available and you don't have to imply that ProjectMixin
has been inherited before.
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.
That makes sense. I guess I was just wary of "incestuous" inheritances. I trust that Python is able to resolve the class family tree perfectly.
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.
If you really want to understand how Python does method resolution for multiple inheritance, https://www.python.org/download/releases/2.3/mro/ is worth reading. There's a bit too much detail there, but there are some examples that will give you the idea.
Hello @oliverroick and @clash99: I have resolved your comments. Please see the updated PR comment for the updated list of changes. I have also added #407 into this PR (use a single "Add" button instead of the 2-set buttons). |
The placement of the add button looks a bit odd in the right-hand panel: Otherwise, it's good to go. Let's see what @clash99 says. |
@oliverroick: I've updated the code to make the placement of the add button on the same row as the search bar if the add button is associated with project data tables. This includes the 4 types of resources data tables, as well as the relationships data table. Please see the following screenshots: |
Good job, thanks! Merging this. |
Proposed changes in this pull request
project_has_resources
context variable to all affected views, where the attribute is true if there are any resources belonging under the projectproject_has_resources
context variable mentioned above), this single "Add" button either links to the "Add from library" modal or the "Upload new" modalScreenshots
Project that has resources in the library
Project that has an empty library
When should this PR be merged
Risks
Follow-up actions