Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bump to primer_view_components 0.28.1 and update PageHeaders in Core #15129
Changes from 39 commits
12c3fd3
24d9a42
8f2b9f2
2b21ffe
9ad93c7
108490d
6a91890
555b141
cf7bf7c
e152956
0c6ae6f
82364e1
c662b50
fb3c6a7
051a53f
eecae54
522ab3f
d622636
0ca05da
1f6185f
978006b
991be14
aab2443
49a06fa
b70ddc5
17996f1
338000b
a7ebc43
ba91ed8
bbf0a01
735e5ef
b1c3932
7bdcfdd
15d619b
4ffa2ff
eea6bf8
b110240
ac68ad8
1a5581b
c60c150
90f0721
2ece709
1a1a3f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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:
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.