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

Refactor sidebar logic into reusable component #1384

Merged
merged 17 commits into from
Mar 19, 2024

Conversation

gromdimon
Copy link
Collaborator

Issue: #1380

@gromdimon gromdimon marked this pull request as draft March 6, 2024 16:09
@gromdimon gromdimon self-assigned this Mar 6, 2024
@gromdimon gromdimon added this to the v1.0.0 milestone Mar 6, 2024
@gromdimon
Copy link
Collaborator Author

It seems, that there's no template tags which can be removed after refactoring. However, I found that projectroles_tags.get_role_import_action is not used anywhere. Is it some kind of an artifact or or it's used somewhere in sodar-server/varfish or I missed something?

btw. this template_tag was not used in _project_sidebar.html

@gromdimon gromdimon marked this pull request as ready for review March 6, 2024 16:41
@mikkonie
Copy link
Collaborator

It seems, that there's no template tags which can be removed after refactoring. However, I found that projectroles_tags.get_role_import_action is not used anywhere. Is it some kind of an artifact or or it's used somewhere in sodar-server/varfish or I missed something?

Nice find. This seems to be an old template tag related to remote project sync which was refactored away, but I guess we forgot to remove the tag. It shouldn't be used anywhere else either, as it's in the projectroles_tags module, not the projectroles_common_tags one. Feel free to remove. It can be done as part of this branch even if not directly related.

Copy link
Collaborator

@mikkonie mikkonie left a comment

Choose a reason for hiding this comment

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

The implementation is working correctly, good work. Here are some comments. There is one major thing I missed in my spec, I will write a separate comment about that and also update the spec.

projectroles/templates/projectroles/_project_sidebar.html Outdated Show resolved Hide resolved
projectroles/utils.py Outdated Show resolved Hide resolved
projectroles/utils.py Show resolved Hide resolved
projectroles/utils.py Outdated Show resolved Hide resolved
projectroles/utils.py Outdated Show resolved Hide resolved
projectroles/utils.py Outdated Show resolved Hide resolved
projectroles/utils.py Outdated Show resolved Hide resolved
projectroles/tests/test_templatetags.py Outdated Show resolved Hide resolved
projectroles/tests/test_templatetags.py Outdated Show resolved Hide resolved
projectroles/utils.py Outdated Show resolved Hide resolved
@mikkonie
Copy link
Collaborator

Ok, so: I completely overlooked _project_menu_btn.html in my spec. It is basically a responsive replacement for the sidebar, containing the exact same links but rendered as a dropdown.

Needless to say, this should also be refactored to use the new helper class instead of having the logic within its template. So here's what should be done:

  1. Refactor _project_menu_btn.html to get links from the helper and render them as is done currently
  2. Remove unused template tags

Copy link
Collaborator

@mikkonie mikkonie left a comment

Choose a reason for hiding this comment

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

This is proceeding nicely, I had some more feedback.

projectroles/templates/projectroles/_project_sidebar.html Outdated Show resolved Hide resolved
projectroles/templatetags/projectroles_tags.py Outdated Show resolved Hide resolved
projectroles/templatetags/projectroles_tags.py Outdated Show resolved Hide resolved
projectroles/utils.py Outdated Show resolved Hide resolved
projectroles/utils.py Show resolved Hide resolved
projectroles/templates/projectroles/_project_sidebar.html Outdated Show resolved Hide resolved
projectroles/tests/test_templatetags.py Outdated Show resolved Hide resolved
projectroles/tests/test_templatetags.py Outdated Show resolved Hide resolved
projectroles/tests/test_templatetags.py Outdated Show resolved Hide resolved
projectroles/tests/test_templatetags.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mikkonie mikkonie left a comment

Choose a reason for hiding this comment

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

Good work! I had just a couple very minor comments, after these I'm happy to merge.

projectroles/templates/projectroles/_project_sidebar.html Outdated Show resolved Hide resolved
projectroles/tests/test_templatetags.py Outdated Show resolved Hide resolved
projectroles/utils.py Outdated Show resolved Hide resolved
@mikkonie mikkonie merged commit 117ec13 into dev Mar 19, 2024
5 checks passed
@mikkonie mikkonie deleted the 1380-refactor-sidebar-logic-into-reusable-component branch March 19, 2024 12:39
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.

None yet

2 participants