Visitar URL original
Identifier & footer external link wrapping in safari by mejiaj · Pull Request #1918 · uswds/uswds-site · GitHub
Skip to content

Identifier & footer external link wrapping in safari - #1918

Merged
thisisdano merged 12 commits into
mainfrom
jm-site-external-link-wrapping
Feb 28, 2023
Merged

Identifier & footer external link wrapping in safari#1918
thisisdano merged 12 commits into
mainfrom
jm-site-external-link-wrapping

Conversation

@mejiaj

@mejiaj mejiaj commented Nov 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

⚠️ uswds/uswds#5046 must be merged before this PR.

Small part relies on uswds/uswds#5046 for the whitespace issue on safari, but has improvements that can be merged.

Content editors

Content for site Identifier and Footer is now done in YML files. Instead of updating template markup you can go to _data/footer.yml and _data/identifier.yml to update content.

Preview links

Tested

I tested the following areas shown below.

Component preview
image

External link guidance section
image

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

@mejiaj mejiaj mentioned this pull request Nov 21, 2022
Merged
6 of 7 tasks
@mejiaj mejiaj changed the title [Test] External link wrapping and safari issues Identifier external link wrapping in safari Dec 2, 2022
@mejiaj
mejiaj requested a review from amyleadem December 2, 2022 14:45
@mejiaj
mejiaj marked this pull request as ready for review December 2, 2022 14:47
amyleadem
amyleadem reviewed Dec 2, 2022
View reviewed changes
Comment thread _includes/identifier.html Outdated
<div class="usa-identifier__container">
<ul class="usa-identifier__required-links-list">
{%- for link in identifier.requiredLinks -%}
<li>

@amyleadem amyleadem Dec 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I noticed that there was a visual difference in the identifier at mobile widths. Looks like the <li> elements might be missing .usa-identifier__required-links-item. All other markup from the original file appears to be present.

image

amyleadem
amyleadem suggested changes Dec 2, 2022
View reviewed changes

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Identifier is looking good. I noticed one missing class (details), but the line break issue seems to be resolved.

I also noticed that the issue still appears on the external links in the footer:
image

Can you take a look at that as well?

@mejiaj mejiaj mentioned this pull request Dec 19, 2022
Closed
2 of 4 tasks
@mejiaj

mejiaj commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

Identifier is looking good. I noticed one missing class (details), but the line break issue seems to be resolved.

I also noticed that the issue still appears on the external links in the footer: image

Can you take a look at that as well?

@amyleadem fixed in a103d4c.

Safari Version 16.1 (18614.2.9.1.12)
image

@mejiaj mejiaj changed the title Identifier external link wrapping in safari Identifier & footer external link wrapping in safari Dec 19, 2022
@mejiaj
mejiaj requested a review from amyleadem December 19, 2022 17:31
amyleadem
amyleadem reviewed Dec 21, 2022
View reviewed changes
Comment thread _includes/footer.html Resolved
amyleadem
amyleadem suggested changes Dec 21, 2022
View reviewed changes

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mejiaj The footer is looking good! I had one question about the usa-link class. I also noticed that the identifier list items are still missing the usa-identifier__required-links-item class. Can you take a look at those?

@mejiaj

mejiaj commented Jan 17, 2023

Copy link
Copy Markdown
Contributor Author

@mejiaj The footer is looking good! I had one question about the usa-link class. I also noticed that the identifier list items are still missing the usa-identifier__required-links-item class. Can you take a look at those?

Added missing class in e8fa6f7. Good catch!

@mejiaj
mejiaj requested a review from amyleadem January 17, 2023 21:25
amyleadem
amyleadem reviewed Jan 17, 2023
View reviewed changes
Comment thread _data/identifier.yml Resolved
amyleadem
amyleadem approved these changes Jan 17, 2023
View reviewed changes

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Everything looks good. Thanks for cleaning this up!

FYI - I committed one small change to add a missing question mark in 4c2b899

⚠️ Also, a reminder to update uswds in the package.json before merging

@mejiaj

mejiaj commented Jan 19, 2023

Copy link
Copy Markdown
Contributor Author

@amyleadem thanks for that and the great review, much appreciated! I've also set the branch to use the latest USWDS.

@mejiaj
mejiaj requested a review from thisisdano January 19, 2023 15:11
thisisdano
thisisdano approved these changes Feb 28, 2023
View reviewed changes

@thisisdano thisisdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice collaboration getting this done!

@thisisdano
thisisdano merged commit 5280d77 into main Feb 28, 2023
@thisisdano
thisisdano deleted the jm-site-external-link-wrapping branch February 28, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@thisisdano thisisdano thisisdano approved these changes
@amyleadem amyleadem amyleadem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

USWDS-Site - Feature: Templatize footer USWDS-Site - Bug: Remove extra whitespace in identifier links

3 participants

@mejiaj @thisisdano @amyleadem