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

Bump to primer_view_components 0.28.1 and update PageHeaders in Core #15129

Merged
merged 43 commits into from
Apr 10, 2024

Conversation

bsatarnejad
Copy link
Contributor

@bsatarnejad bsatarnejad commented Mar 27, 2024

With version 0.26.0 of our primer_view_components, the definition of the Primer::OpenProject::PageHeader component has changed. So we have to update those places where it is already used.

Places to update:

  • Members page
  • Projects list page
  • Admin Project Attributes pages
  • Quarantined attachments page
  • Virus scanning page
  • Storages pages

https://community.openproject.org/work_packages/52746/activity

a related bugfix: https://community.openproject.org/projects/openproject/work_packages/53663/activity

Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Hi @bsatarnejad

I now that this is still in a Draft state, but I had a look already as I had some time at hand.

Gemfile Show resolved Hide resolved
frontend/package.json Show resolved Hide resolved
@HDinger HDinger changed the title Bump to primer_view_components 0.26.0 and update PageHeaders in Core Bump to primer_view_components 0.27.0 and update PageHeaders in Core Apr 4, 2024
@HDinger HDinger marked this pull request as ready for review April 4, 2024 13:15
@HDinger HDinger force-pushed the implementation-update-page-header-in-core branch 2 times, most recently from 75aa9f4 to b110240 Compare April 5, 2024 07:54
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

The look of it is nice. I in particular like the mobile behaviour.

There are a couple of remarks on the code that need to be addressed before merging. Of those, the one about the CSP is I guess the most troublesome. I don't think it originates in this PR but it is something we need to fix before the release.

align_self: :center,
"aria-label": I18n.t("button_save_as"),
mr: 1)
) + content_tag(:span, t("button_save_as")) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

It would read easier if one could say something like:


header.with_action_link(**options) do |link|
  link.with_leading_visual_icon('op-save')
  t('button_save_as')
end  

or have the text and the icon be part of the with_action_link method. But that does not seem to be along the line of thinking of how Primer components are build.

What also strikes me as weird is that a link is used and not a button. This seems to violate the statement of primer: "Links navigate you to a new place or new content. Do not use links for triggering an action. Instead, use buttons, which are designed to activate a feature." (source)

Copy link
Contributor

Choose a reason for hiding this comment

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

header.with_action_link(**options) do |link|
link.with_leading_visual_icon('op-save')
t('button_save_as')
end

The link component unfortunately does not support leading visual (although they mention it in the docs, it is not implemented).

The product team and I discussed the decision for the link for quite some time. Of course, the invisible button would be the best solution. However, Primer has this weird behavior of changing the font color of that buttons depending on the existance of an icon. The issue I raised for that in Primer was closed witht the comment that it "is working as expected" 🤷 Since GitHub and even Primer itself is very fuzzy when it comes to their usage of Buttons vs Links, we decided to go with the Link at this place.

app/components/projects/index_page_header_component.rb Outdated Show resolved Hide resolved
@HDinger HDinger changed the title Bump to primer_view_components 0.27.0 and update PageHeaders in Core Bump to primer_view_components 0.28.1 and update PageHeaders in Core Apr 10, 2024
@HDinger HDinger requested a review from ulferts April 10, 2024 09:56
@HDinger HDinger merged commit 5fdaf52 into dev Apr 10, 2024
10 checks passed
@HDinger HDinger deleted the implementation-update-page-header-in-core branch April 10, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants