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

3857 breadcrumb remove currentpage href #3861

Merged
merged 22 commits into from
Apr 16, 2024

Conversation

StephDriver
Copy link
Contributor

@StephDriver StephDriver commented Jan 5, 2024

I have worked through the known areas of this issue one directory at a time, and each has its own commit on this branch. Then there will be a final commit coming which tidies up any typos and trailing tags that were missed.

The breadcrumbs have been implemented in three different ways -

  1. some were fully within the template itself,
  2. some used an include to a breadcrumb template within elements/breadcrumbs,
  3. and some used the include while listing the current page after it within the main template.

This fix was about making sure that final item in the list was not a link.

For 1. there was a simple fix of removing the <a> tags from the final item.

For 2 and 3, Where an include was used, it needed to work for templates that only used the include, and those which had a final line in the template - i.e. sometimes return a list of links, and sometimes a list where the last item was not a link. some of the time the problem was within the include, and then within the same section there might be some templates which used the include and then listed the current page with or without tags in the main template. Fixing this across each section required the some includes to be updated and some of the main templates, and some templates then worked without modification due to the updated includes. Where there was a subpage listed within the main template, I have updated the includes with the suffix with subpage="Yes"so that the include can then display all items as links.

This is mostly done, but given how many changes were made, I will now review the changes on github so that I can check for any typos or trailing </a> that have not been caught already.

@StephDriver StephDriver marked this pull request as draft January 5, 2024 10:26
@StephDriver
Copy link
Contributor Author

4 typos caught and that's done.

Closes #3857

@StephDriver StephDriver marked this pull request as ready for review January 5, 2024 10:40
@StephDriver StephDriver linked an issue Jan 22, 2024 that may be closed by this pull request
Copy link
Member

@joemull joemull 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 great--thank you for going through all these so carefully. I've checked about half of them manually and generally things are working really well. I think it's very close to ready to be merged.

I noticed a few tiny things that must have been bugs before your changes, but they jumped out to me in reviewing your changes. Also there's one place that the template variables for user names need to be different, because of some inconsistency in how we set them in the views. All the details in comments inline.

src/templates/admin/cms/submission_item_list.html Outdated Show resolved Hide resolved
src/templates/admin/core/manager/users/edit.html Outdated Show resolved Hide resolved
@joemull
Copy link
Member

joemull commented Mar 7, 2024

One other thing I forgot to mention: because we use the typesetting plugin as a normal part of the standard workflow (and are planning to integrate it) we should at some point soon make the same changes there so that the workflow is consistent.

@StephDriver StephDriver self-assigned this Apr 5, 2024
@StephDriver StephDriver added a11y Issues that relate to acessibility and removed a11y Issues that relate to acessibility labels Apr 12, 2024
@StephDriver
Copy link
Contributor Author

changes made - please re-review

@StephDriver StephDriver requested review from joemull and removed request for ajrbyers April 12, 2024 10:42
@StephDriver StephDriver removed their assignment Apr 12, 2024
@joemull joemull requested a review from ajrbyers April 15, 2024 13:03
@joemull joemull assigned ajrbyers and unassigned joemull Apr 15, 2024
Copy link
Member

@ajrbyers ajrbyers 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 great, I've added two comments for you to go over.

src/templates/admin/journal/manage/issue_add_article.html Outdated Show resolved Hide resolved
src/templates/admin/submission/manager/delete_license.html Outdated Show resolved Hide resolved
@ajrbyers ajrbyers assigned StephDriver and unassigned ajrbyers Apr 15, 2024
@StephDriver StephDriver requested a review from ajrbyers April 16, 2024 13:22
@StephDriver StephDriver assigned ajrbyers and unassigned StephDriver Apr 16, 2024
@ajrbyers ajrbyers merged commit de81629 into master Apr 16, 2024
1 check was pending
@ajrbyers ajrbyers deleted the 3857-breadcrumb_remove_currentpage_href branch April 16, 2024 13:25
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.

many breadcrumbs displaying current page as link
3 participants