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

Add support for trailing and leading visual icons in Primer::Beta::Link #2982

Conversation

HDinger
Copy link
Contributor

@HDinger HDinger commented Jul 30, 2024

What are you trying to accomplish?

This PR adds slots for a trailing_visual and a leading_visual. As of now, only Icons are supported as types.

Screenshots

Bildschirmfoto 2024-07-30 um 14 19 42 Bildschirmfoto 2024-07-30 um 14 19 37

List the issues that this change affects.

Closes #2981

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?

I limited the visual to icons for now, as I did not know, which other types (avatar, svg, ...) shall be supported.

Anything you want to highlight for special attention from reviewers?

I extracted the code of the call method to a html.erb template because of the increased complexity of the template. In there I used a capture block to avoid the previous duplication depending on the existance of a tooltip.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@HDinger HDinger requested a review from a team as a code owner July 30, 2024 12:32
@HDinger HDinger requested a review from owenniblock July 30, 2024 12:32
Copy link

changeset-bot bot commented Jul 30, 2024

🦋 Changeset detected

Latest commit: 681670a

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

This PR includes changesets to release 1 package
Name Type
@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

@HDinger
Copy link
Contributor Author

HDinger commented Aug 1, 2024

One of the failing tests is checking that there is no leading and trailing white space before the link text. Currently, every linebreak in the erb file is interpreted as white space. The erb syntax allows to strip the trailing white space with -%>. However for leading <%= this is not possible, see the docs.

I currently see two options:

  1. Wrapp a span around the content like it is done for buttons.
  2. Write the whole link_content in the erb in one line. That would look ugly as hell, but leaves the DOM unchanged.

Let me know what you prefer.

@HDinger HDinger force-pushed the code-maintenance/56822-add-support-for-trailing-and-leading-icons-for-link branch from 06a78ea to ac5347e Compare August 1, 2024 11:06
Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Thanks for this 😄

app/components/primer/beta/link.html.erb Outdated Show resolved Hide resolved
app/components/primer/beta/link.html.erb Outdated Show resolved Hide resolved
app/components/primer/beta/link.html.erb Outdated Show resolved Hide resolved
@camertron
Copy link
Contributor

One of the failing tests is checking that there is no leading and trailing white space

Ah yeah I see what you mean... what if we added a trim: boolean argument to BaseComponent (and ConditionalWrapper) that would automatically call #strip on content before emitting it?

@HDinger HDinger force-pushed the code-maintenance/56822-add-support-for-trailing-and-leading-icons-for-link branch 3 times, most recently from 04c9c94 to 8c523be Compare August 28, 2024 11:06
… the content with `strip` before rendering it
@HDinger HDinger force-pushed the code-maintenance/56822-add-support-for-trailing-and-leading-icons-for-link branch from 8c523be to b0e8b11 Compare August 28, 2024 11:17
@HDinger
Copy link
Contributor Author

HDinger commented Aug 28, 2024

Hi @camertron

I adressed your remarks and introduced a trim flag which calls strip on the content. The component tests are now green. There are three other tests failing, which I really don't know why. Especially the changeset tests is weird, as there is a changeset added 🤷

app/components/primer/component.rb Outdated Show resolved Hide resolved
app/components/primer/conditional_wrapper.rb Outdated Show resolved Hide resolved
app/components/primer/base_component.rb Outdated Show resolved Hide resolved
@camertron
Copy link
Contributor

the changeset tests is weird, as there is a changeset added 🤷

Ah yeah, that one always fails for forks because it pulls the original repo instead of the forked repo and can't find the branch. It's ok, I can manually override when I merge 👍

@lesliecdubs lesliecdubs requested review from jonrohan and removed request for owenniblock and jonrohan August 28, 2024 18:58
…gests. The flag was moved out and is checked before the method is called
Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for your work on this and for persevering through my feedback! 🎉

@camertron camertron merged commit b5b0199 into primer:main Aug 29, 2024
34 of 38 checks passed
@primer primer bot mentioned this pull request Aug 29, 2024
camertron added a commit that referenced this pull request Aug 29, 2024
camertron added a commit that referenced this pull request Aug 29, 2024
HDinger added a commit to opf/primer_view_components that referenced this pull request Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Primer::Beta::Link is missing support for trainling and leading visuals
2 participants