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

updating action-links styling for proper long text wrap #908

Merged
merged 4 commits into from
Mar 8, 2023

Conversation

oleksii-morgun
Copy link
Contributor

@oleksii-morgun oleksii-morgun commented Mar 2, 2023

Description

Long text in an action link would wrap underneath an icon, particularly the case on mobile view where a screen width is narrow.
Related to the following issue department-of-veterans-affairs/vets-design-system-documentation#1590

Testing done

Only tested within vets-website. Not familiar with any other testing methods here. If someone from design team could please test on your end.

Screenshots

Before
image

After
image
image

Acceptance criteria

  • [ ]

Definition of done

  • Changes have been tested in vets-website
  • Changes have been tested in IE11, if applicable
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

@oleksii-morgun oleksii-morgun marked this pull request as ready for review March 6, 2023 15:56
@oleksii-morgun oleksii-morgun requested a review from a team as a code owner March 6, 2023 15:56
height: 0px; // So the height doesn't mess with the focus halo
float: left;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be better achieved using flexbox? There was a similar issue with radio button labels that was solved that way: department-of-veterans-affairs/component-library#633

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamigibbs I am applying display:flex to <a> tag similar to example you provided. The reason I had to add float:left attribute in ::before is because I could not find any other way around removing underlining of an icon. Do you have any suggestions here?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining and for the screenshot! I can see what you mean now. Without making structural changes, I'm not sure this can be accomplished another way either (I'm not sure why text-decoration: none isn't working in this case).

Copy link
Contributor

@jamigibbs jamigibbs left a comment

Choose a reason for hiding this comment

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

Before merging, do you mind adding a patch bump to the Formation version?

@oleksii-morgun
Copy link
Contributor Author

I am guessing it just bumps up to 7.0.5 here, correct?

@jamigibbs
Copy link
Contributor

I am guessing it just bumps up to 7.0.5 here, correct?

That looks good! Thank you!

@oleksii-morgun oleksii-morgun merged commit 55a3b4a into master Mar 8, 2023
@oleksii-morgun oleksii-morgun deleted the action-link-text-wrap branch March 8, 2023 01:42
@oleksii-morgun
Copy link
Contributor Author

@jamigibbs would you guys need to do any testing on your end, or should I just update the formation version on vet-website as well?

@jamigibbs
Copy link
Contributor

jamigibbs commented Mar 8, 2023

@oleksii-morgun-va I have something else I need to get added to Formation that I should be able to merge today. I'm happy to take on the task of publishing the package and updating vets-website once I have that in.

@jamigibbs
Copy link
Contributor

@oleksii-morgun-va FYI

@Mottie
Copy link
Contributor

Mottie commented Mar 16, 2023

It looks like this change messed up the 526 action link:
green action link icon with the text of 'start a disability compensation claim' displaying underneath the icon so that the word 'start' is only partially visible

But it's because the link had some extra CSS

a.vads-c-action-link--green {
  display: block;
  line-height: 1em;
}

@jamigibbs
Copy link
Contributor

@Mottie Where is that style coming from? Are you saying that we need to re-open the original issue?

@Mottie
Copy link
Contributor

Mottie commented Mar 16, 2023

It was styling that was already in the CSS (for almost a year)... there's already a PR to fix it.

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.

None yet

3 participants