feat[#50499]: Add back button and breadcrumbs to page header #45

dominic-braeunlein · 2023-10-26T09:18:40Z

Work package: https://community.openproject.org/wp/50947

Needed for implementing: https://community.openproject.org/wp/50499
Related PR in OP: opf/openproject#13962

What are you trying to accomplish?

This PR adds breadcrumbs and back button support to the PageHeader.

A breadcrumbs item can be:

An object with href and text e.g. {href: "/admin", text: "Admin" }.
An Anchor-tag string wich can be the return value from breadcrumb_paths method in BreadcrumbHelper e.g. "\u003ca href=\"/root/sub\"\u003eSub\u003c/a\u003e" .
Or a text which is useful for the last element / current url e.g. "Page 2"

This feature also adds an optional show_breadcrumb parameter to deactivate rendering of the breadcrumbs. Which helps harmonisation with the existing BreadcrumbHelper #show_local_breadcrumb control method to maintain backwards compatibility.

e.g.

breadcrumb_items = [
  "test1",
  "test2" ,
  "test3"
]
render(Primer::OpenProject::PageHeader.new) do |header|
  header.with_title() { "Hello" }
  header.with_back_button(href: "#")
  header.with_breadcrumbs(breadcrumb_items, show_breadcrumb: false)
end

Screenshots

List the issues that this change affects.

https://community.openproject.org/wp/50499

Risk Assessment

Low risk the change is small, highly observable, and easily rolled back.
Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

Fixes axe scan violation - This change fixes an existing axe scan violation.
No new axe scan violation - This change does not introduce any new axe scan violations.
New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

changeset-bot · 2023-10-26T09:18:43Z

🦋 Changeset detected

Latest commit: 6ce609d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@openproject/primer-view-components	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

akabiru

Looks good to me! 🎉 I left a few remarks but would leave to @HDinger for final approval 🥇

app/components/primer/open_project/page_header.rb

previews/primer/open_project/page_header_preview.rb

HDinger

Hi @dominic-braeunlein

Looks good 👍 I have some remarks in the code, some of them we might want to discuss.

Further I noticed that the arrow is not correctly aligned with the header text:

At least some of the CI failures are due to missing specs. So please add tests for the newly added features. You can add them here. Let me know, if you need help with the other failing tests.

app/components/primer/open_project/page_header.rb

static/classes.json

app/components/primer/open_project/page_header.rb

…faults

…ckButton

…crumbs

HDinger

Hi @dominic-braeunlein

Nice job, I only have minor remarks. 👍 Further @bsatarnejad noticed that on mobile the text is not wrapping correctly around the arrow:

I'd expect the arrow to stay on the right side, and the text to wrap after it. What do you think?

app/components/primer/open_project/page_header.rb

app/components/primer/open_project/page_header.pcss

… button

…dcrumbs

dominic-braeunlein · 2023-11-07T13:53:09Z

Hi @dominic-braeunlein

Nice job, I only have minor remarks. 👍 Further @bsatarnejad noticed that on mobile the text is not wrapping correctly around the arrow:
I'd expect the arrow to stay on the right side, and the text to wrap after it. What do you think?

Hi @HDinger,
thank you! I went for following solution to wrap the title:

I had to change the HTML structure in the PageHeader to achieve this result, which resulted also in CSS changes. Please have a look and let me know if it is ok to have a div in the template.

While I was changing the CSS I saw that the order attributes could be removed now. Please double check this too, maybe I am missing something.

HDinger

Hi @dominic-braeunlein

I had to change the HTML structure in the PageHeader to achieve this result, which resulted also in CSS changes. Please have a look and let me know if it is ok to have a div in the template.

That is totally okay 👍

While I was changing the CSS I saw that the order attributes could be removed now. Please double check this too, maybe I am missing something.

Yes, seems to work fine 👍

The component looks good to me. I have some minor remarks with regards to the preview.
Further, I had a look on the figma specification and saw that there are some special mobile styles: The back button is hidden, as well as the breadcumb and a different navigation element is shown. In Primer, this is named the context bar of the page header. We can add that in a separate PR to get this one done, but need to keep in mind, that we hide the backButton and breadcrumbs.

previews/primer/open_project/page_header_preview/actions.html.erb

previews/primer/open_project/page_header_preview.rb

dominic-braeunlein · 2023-11-08T12:57:15Z

... and saw that there are some special mobile styles: The back button is hidden, as well as the breadcumb and a different navigation element is shown. In Primer, this is named the context bar of the page header. We can add that in a separate PR to get this one done, but need to keep in mind, that we hide the backButton and breadcrumbs.

Hi @HDinger
Perfect, I will create a new PR for that.

After I added the new parameters to the playground I saw a small misalignment:
The back button (even when set to large) doesn't align perfectly when the title is set to large .
I would look further into this if you think it is necessary. I just didn't find an obvious fix for it. What do you think?

Also, thank you for all your input!

HDinger · 2023-11-09T07:10:34Z

After I added the new parameters to the playground I saw a small misalignment:
The back button (even when set to large) doesn't align perfectly when the title is set to large .
I would look further into this if you think it is necessary. I just didn't find an obvious fix for it. What do you think?

Yeah, I would like to have that solved. I can support you with this, if you like.

…misalignment which we have to cancel out. This is most likely coming from the arrow being thinner than the font

HDinger

Good to merge 👍 Thanks for all the effort @dominic-braeunlein !

github-actions · 2023-11-10T10:03:59Z

Uh oh! @dominic-braeunlein, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

github-actions · 2023-11-10T10:04:14Z

Uh oh! @dominic-braeunlein, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

feat[#50499]: Add back button and breadcrumbs to page header

fcb3934

dominic-braeunlein requested review from HDinger and akabiru October 26, 2023 09:18

github-actions bot added ruby css minor release labels Oct 26, 2023

akabiru reviewed Oct 30, 2023

View reviewed changes

HDinger requested changes Oct 30, 2023

View reviewed changes

dominic-braeunlein added 14 commits October 31, 2023 13:57

chore[#50499]: Set renders_one BackButton attributes size and icon de…

ee8a1dd

…faults

fix[#50499]: Remove I18n from PageHeader

648d4fc

fix[#50499]: Rename CSS class PageHeader-back_button to PageHeader-ba…

3d70f16

…ckButton

fix[#50499]: Fix RuboCop errors and warnings

017bbda

fix[#50499]: Vertically center back button

5bf4dae

fix[#50499]: Add aria-label in preview for with_back_button

fee9a1a

test[#50499]: Add renders_back_button test

4a96f5f

test[#50499]: Add renders_breadcrumbs test

e3da4f1

fix[#50499]: Fix class names in PageHeader

59e5324

chore[#50499]: Remove show_breadcrumb option in PageHeader.with_bread…

3f91140

…crumbs

chore[#50499]: Remove manually added static classes

67b763f

feat[#50499]: Support **system_arguments in PageHeader breadcrumbs slot

83513c4

fix[#50499]: Readd @Label With actions in PageHeaderPreview

68f12e0

fix[#50499]: Fix spaces

33a363d

dominic-braeunlein mentioned this pull request Nov 1, 2023

feat[#50499]: Add navigational elements (breadcrumb and back buttons) opf/openproject#13962

Merged

HDinger requested changes Nov 2, 2023

View reviewed changes

app/components/primer/open_project/page_header.rb Show resolved Hide resolved

app/components/primer/open_project/page_header.rb Show resolved Hide resolved

app/components/primer/open_project/page_header.pcss Outdated Show resolved Hide resolved

dominic-braeunlein added 5 commits November 7, 2023 12:28

fix[#50499]: Change PageHeader align-items to baseline

b5069e0

fix[#50499]: Add size and icon options and use fetch_or_fallback

76f469d

chore[#50499]: Add params options in preview for PageHeader with back…

4f57dc1

… button

chore[#50499]: Add params information in back_button slot

18e5c94

chore[#50499]: Add params options in preview for PageHeader with brea…

2f4ad11

…dcrumbs

dominic-braeunlein added 2 commits November 7, 2023 15:46

chore[#50499]: Add anchor_tag_string helper function

6db5989

fix[#50499]: Fix back button and title wrapping on small screen

cdc8973

HDinger reviewed Nov 8, 2023

View reviewed changes

previews/primer/open_project/page_header_preview/actions.html.erb Outdated Show resolved Hide resolved

previews/primer/open_project/page_header_preview.rb Outdated Show resolved Hide resolved

previews/primer/open_project/page_header_preview.rb Outdated Show resolved Hide resolved

dominic-braeunlein added 5 commits November 8, 2023 17:51

chore[#50499]: Remove long testing title from preview

4f0227e

chore[#50499]: Remove back button in breadcrumb preview

5f79542

chore[#50499]: Sort PageHeader with-alternatives alphabetically

a0afd49

chore[#50499]: Add toggles for back button and breadcrumbs in playground

7ee873c

chore[#50499]: Add back button size parameter to playground

dbfdaf9

opf deleted a comment from github-actions bot Nov 8, 2023

Center back button in any cases. Unfortunately, there is a two pixel …

6ce609d

…misalignment which we have to cancel out. This is most likely coming from the arrow being thinner than the font

HDinger approved these changes Nov 9, 2023

View reviewed changes

HDinger merged commit ac9f727 into main Nov 9, 2023
25 checks passed

HDinger deleted the add-back-and-breadcrumbs-to-page-header branch November 9, 2023 09:21

openprojectci mentioned this pull request Nov 9, 2023

Release Tracking #47

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[#50499]: Add back button and breadcrumbs to page header #45

feat[#50499]: Add back button and breadcrumbs to page header #45

dominic-braeunlein commented Oct 26, 2023 •

edited by wielinde

Loading

changeset-bot bot commented Oct 26, 2023 •

edited

Loading

akabiru left a comment

HDinger left a comment •

edited by dominic-braeunlein

Loading

HDinger left a comment

dominic-braeunlein commented Nov 7, 2023

HDinger left a comment

dominic-braeunlein commented Nov 8, 2023

HDinger commented Nov 9, 2023

HDinger left a comment

github-actions bot commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

feat[#50499]: Add back button and breadcrumbs to page header #45

feat[#50499]: Add back button and breadcrumbs to page header #45

Conversation

dominic-braeunlein commented Oct 26, 2023 • edited by wielinde Loading

What are you trying to accomplish?

Screenshots

List the issues that this change affects.

Risk Assessment

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

Merge checklist

changeset-bot bot commented Oct 26, 2023 • edited Loading

🦋 Changeset detected

akabiru left a comment

Choose a reason for hiding this comment

HDinger left a comment • edited by dominic-braeunlein Loading

Choose a reason for hiding this comment

HDinger left a comment

Choose a reason for hiding this comment

dominic-braeunlein commented Nov 7, 2023

HDinger left a comment

Choose a reason for hiding this comment

dominic-braeunlein commented Nov 8, 2023

HDinger commented Nov 9, 2023

HDinger left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

dominic-braeunlein commented Oct 26, 2023 •

edited by wielinde

Loading

changeset-bot bot commented Oct 26, 2023 •

edited

Loading

HDinger left a comment •

edited by dominic-braeunlein

Loading