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

feat(relations): list relations under tabs #1276

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gythaogg
Copy link
Contributor

@gythaogg gythaogg commented Oct 8, 2024

This pull request includes an update to the list_relations_include.html template to enhance the user interface by switching from a card layout to a tabbed navigation layout.

UI Improvements:

closes #489

TO DISCUSS

  • Potentially useful to have an ALL tab to have a quick overview and set that as default. This is not implemented in this pull request.
  • TibSchol users would prefer the tab headings to also show the number of relations under each tab so they don't have to cycle through all the tabs to see which one has content. Don't know if it is worth it (it was only a nice-to-have wish).
  • When you are in a particular tab and then do an action that causes the page to reload (by clicking on Edit, for example) then the first tab will be active again.

When new relations are created, they should be added to the correct element so that they are not displayed under all tabs

After new relations are created, they appear correctly in the active tab but need a refresh for them to appear in the ALL tab.

@gythaogg gythaogg requested a review from b1rger October 8, 2024 13:27
@gythaogg gythaogg marked this pull request as draft October 9, 2024 08:08
Copy link
Contributor

@b1rger b1rger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! We should discuss in JFX if this should be the default. An "ALL" tab would definitly make sene.

Regarding the problem with all relations being listed after a change: do you already know what we need to change to fix that? Mabye we have to refactor the list_relations.html template a bit and pass the target as an argument...

@gythaogg
Copy link
Contributor Author

gythaogg commented Oct 10, 2024

Regarding the problem with all relations being listed after a change: do you already know what we need to change to fix that? Mabye we have to refactor the list_relations.html template a bit and pass the target as an argument...

Didn't look into the code, but from HTML inspection I noticed that the new relations are in the correct div (as in they have the expected id element) but missing the necessary classes (tab-pane, active) to make them tab content. But crucially I noticed just now that I am using the same ID for my tab container that you are using in the div to encapsulate list of relations with a target group 🤦‍♀️

Fixed this bug and now new relations are getting added in the correct place automatically. Will force push now.

@gythaogg gythaogg force-pushed the gythaogg/489-relations-list-tabs branch 3 times, most recently from 120c40e to 035b1aa Compare October 10, 2024 08:10
@gythaogg gythaogg added the needs-attention This issue or pull request is in need of discussion, information, assessment by team members label Oct 10, 2024
@gythaogg gythaogg marked this pull request as ready for review October 10, 2024 08:27
@gythaogg gythaogg self-assigned this Oct 11, 2024
@sennierer
Copy link
Collaborator

look into keeping both options
also see if anchor elements plus scrolling and rolling headers makes sense
also sorting of tables needs to be sorted

@gythaogg gythaogg force-pushed the gythaogg/489-relations-list-tabs branch from 035b1aa to 62100cd Compare October 15, 2024 12:33
@gythaogg gythaogg marked this pull request as draft October 15, 2024 12:37
@gythaogg gythaogg force-pushed the gythaogg/489-relations-list-tabs branch 2 times, most recently from 4dc44f2 to 3f75d7f Compare October 15, 2024 13:31
@gythaogg
Copy link
Contributor Author

@b1rger I tried to override the person template to use the tabbed view of relations for sample_project - 3f75d7f

It looks like it displays the relations in tabs and via the block.super call, it also displays the relations in the grouped list format and I can't quite figure out how to skip the overridden template in relations/templates/apis_core/apis_entities/... and only run the super code in apis_core/apis/entities/

View
image

Edit
image

Am I doing something wrong here?

@b1rger
Copy link
Contributor

b1rger commented Oct 15, 2024

@b1rger I tried to override the person template to use the tabbed view of relations for sample_project - 3f75d7f

It looks like it displays the relations in tabs and via the block.super call, it also displays the relations in the grouped list format and I can't quite figure out how to skip the overridden template in relations/templates/apis_core/apis_entities/... and only run the super code in apis_core/apis/entities/

Sorry, I'm not quite sure what you want to achieve. Why don't you drop the {{ block.super }}?

@gythaogg
Copy link
Contributor Author

Sorry, I'm not quite sure what you want to achieve. Why don't you drop the {{ block.super }}?

Because your template (https://github.com/acdh-oeaw/apis-core-rdf/blob/main/apis_core/relations/templates/apis_core/apis_entities/abstractentity_detail.html) invokes {{ block.super }} - and in the future if core provides something else in col-one I would expect the tabs template to show it just like the default relations template would. Does this make sense?

I am thinking that it would work if the relations table had their own block that gets overridden by the different templates....

@gythaogg gythaogg force-pushed the gythaogg/489-relations-list-tabs branch from 3f75d7f to 07139e1 Compare October 16, 2024 08:09
@gythaogg
Copy link
Contributor Author

look into keeping both options

Modified PR to provide tabbed listing as a separate template. Used in sample project Person (Place and Group use the default listing)

also see if anchor elements plus scrolling and rolling headers makes sense

Anchor elements/scrolling - will implement as part of another PR

also sorting of tables needs to be sorted

The current templates to list relations - both default and tabbed do not contain a header column
Default template so I will leave it out of this pull request.

image

@gythaogg gythaogg marked this pull request as ready for review October 16, 2024 08:23
@gythaogg gythaogg marked this pull request as draft October 16, 2024 08:24
@gythaogg gythaogg force-pushed the gythaogg/489-relations-list-tabs branch from 07139e1 to 991fd42 Compare October 16, 2024 08:39
@gythaogg gythaogg marked this pull request as ready for review October 16, 2024 08:41
@gythaogg gythaogg requested a review from b1rger October 16, 2024 08:41
Copy link
Contributor

@b1rger b1rger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please move the introduction of the relations-include block into a separate PR
  • I'm not sure what the "To Be Done" in one of the commit messges means - does it mean that this PR is still a draft and those things will be implemented? Or should those todos be implemented after the PR is merged?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change for?

Copy link
Contributor Author

@gythaogg gythaogg Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change if I override col-one in my ontology (to point to the tabs relation template,- in the inheritance chain the call to block.super includes the col-one from the templates in relations first - which includes default listing of relations.

This allows me to only override the part that includes the relations template within col-one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please move the introduction of the relations-include block into a separate PR

Considering I cannot override the relations template correctly (i.e. with the default and the alternative horizontal tab template both including block.super call in col-one), I think it should be part of this PR. But if you strongly feel that it doesn't belong here, please convince me too.

  • I'm not sure what the "To Be Done" in one of the commit messges means - does it mean that this PR is still a draft and those things will be implemented? Or should those todos be implemented after the PR is merged?

Oops that was meant for me - shouldn't have sent it with the PR for review. It's now removed, thanks!

@gythaogg gythaogg force-pushed the gythaogg/489-relations-list-tabs branch from 991fd42 to 10b3a52 Compare October 22, 2024 05:58
@b1rger
Copy link
Contributor

b1rger commented Oct 22, 2024

Considering I cannot override the relations template correctly (i.e. with the default and the alternative horizontal tab template both including block.super call in col-one), I think it should be part of this PR. But if you strongly feel that it doesn't belong here, please convince me too.

Maybe a quote from a recent article can convince you:

Example: say I’m working on a feature that changes how user settings are displayed in the UI. While working on that, I notice that I need to change how user settings are parsed. It’s a two-line change. I will put that two-line change in a separate PR, separate from the UI changes even if that’s where I found the need to make the change. Why? Because if two days later someone says “something’s wrong with our settings parser”, I want to be able to directly point to either the UI change or the parsing change and revert one or the other.

Additionally, the 2-line change of adding the relations-include block would already be merged by now.

@b1rger
Copy link
Contributor

b1rger commented Oct 22, 2024

Without this change if I override col-one in my ontology (to point to the tabs relation template,- in the inheritance chain the call to block.super includes the col-one from the templates in relations first - which includes default listing of relations.

I'm not sure if we talk about the same change. I'm talking about the addition of the ALL if no target exists.

@gythaogg
Copy link
Contributor Author

I'm not sure if we talk about the same change. I'm talking about the addition of the ALL if no target exists.

Yep I misunderstood.

This change checks if a specific target group exists because, when this file is included under the ALL tab target is not set. target.name will throw an error if target is None.

The template function relations_list_table doesn't mind that target is None, it returns all the relations in that case.

So along the lines of your earlier comment - do you think I should treat this as a separate "bug" even though if I can't reproduce the bug without this PR?

@sennierer sennierer removed the needs-attention This issue or pull request is in need of discussion, information, assessment by team members label Oct 22, 2024
@gythaogg gythaogg marked this pull request as draft October 22, 2024 12:25
@gythaogg gythaogg force-pushed the gythaogg/489-relations-list-tabs branch 3 times, most recently from 2f02f56 to 5db43c6 Compare October 22, 2024 12:36
gythaogg and others added 2 commits October 22, 2024 14:47
Displays the relations in a tabbed view with ALL tab appearing in the
beginning, followed by the other target groups.

closes #489
Person - view and edit pages list relations in a tabbed format by
overriding relations-include block in col-one
@gythaogg gythaogg force-pushed the gythaogg/489-relations-list-tabs branch from 5db43c6 to 42c2e7d Compare October 22, 2024 12:48
@gythaogg gythaogg requested a review from b1rger October 22, 2024 13:18
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.

Redesigning relations list view for entity
3 participants