feat: Semantic HTML Audit by changangus · Pull Request #32 · sparkbox/apprentices-page-next

changangus · 2021-12-06T18:20:57Z

Description

Apprentice cmp
- article to div
- h2 to h3
- pass name prop to SocialLinks for link aria labels
- update test to reflect new alt text for images
Apprentice Status cmp
- Add visually hidden span that will act as label for employee status
Link cmp
- Use name prop to create dynamic aria labels for a tag
- update tests
Previous Apprentice cmp
- article to section
- section to div
- add z-index to scss module (padding was covering current apprentice links)
Previous Apprentice Group cmp
- p to h3
- add aria label to label apprentice group version
SocialLinks cmp
- take passed in name prop and pass it into Link cmp
- update tests
Home Page
- move hero outside of main tag
Updated snapshot

FSA21V2-163

Spec

See Story: FSA21V2-163

To Test

Make sure all PR Checks have passed (Github Actions, Netlify etc).
Pull down all related branches.
Confirm all tests pass: npm run test:ci

netlify · 2021-12-06T18:21:03Z

✔️ Deploy Preview for sb-apprentices ready!

🔨 Explore the source changes: 6d134ee

🔍 Inspect the deploy log: https://app.netlify.com/sites/sb-apprentices/deploys/61b38b039914bd00071a21fc

😎 Browse the preview: https://deploy-preview-32--sb-apprentices.netlify.app

jonoliver · 2021-12-07T16:19:27Z

components/link/link.js

-const Link = ({ href, children }) => (
-  <a className={styles.link} href={href}>{children}</a>
+const Link = ({ href, name, children }) => (
+  <a className={styles.link} href={href} aria-label={`${name}'s ${children}'`}>{children}</a>


Since this is a generic Link component, as opposed to something with a more specific purpose (eg a single SocialLink component), I don't think that name is the appropriate level of abstraction here. I think the prop should just be aria-label, and the current logic for formatting that string lifted from the component into the parent.

In this case, it looks like this is only used in the context of SocialLinks, so I think the string formatting can go directly in that component. Hypothetically, if we did need to reuse a component that included the name formatting, we could use composition: a SocialLink component which accepts the name, then passes the formatted name to the underlying generic Link component.

Sounds good to me

jonoliver · 2021-12-07T16:21:12Z

components/apprentice-status-indicator/apprentice-status-indicator.module.scss

+
+.status-label {
+  @include visually-hidden;
+}


Just making a note: Let's be sure to coordinate this merge with #31 since it includes the extracted mixin

catheraaine · 2021-12-08T16:59:09Z

components/apprentice-status-indicator/apprentice-status-indicator.js

-      />
-    </svg>
+    <>
+      <span className={styles['status-label']}>


catheraaine · 2021-12-08T17:01:37Z

components/link/link.test.js

+    render(<Link href="http://some/url" name="name">hello</Link>);
    expect(screen.getByRole('link')).toHaveTextContent('hello');
  });
 });


Is it possible to test the new label?

catheraaine

LGTM, once Jon checks that change he requested, I'm good if he wants to approve.

catheraaine · 2021-12-08T17:02:00Z

components/previous-apprentices-group/previous-apprentices-group.js

-    <p
+    <h3
      className={styles['previous-apprentices-group__version']}
+      aria-label={`Previous apprentices group version ${version}`}


I love this.

catheraaine · 2021-12-08T17:03:50Z

pages/index.js

      <link rel="icon" href="/favicon.ico" />
    </Head>
-
+    <Hero />


Nice, easy fix

jonoliver · 2021-12-09T17:57:46Z

components/social-links/social-links.test.js

+      name="name"
    />);
    const links = screen.getAllByRole('link');
    expect(links[0]).toHaveAttribute('href', 'https://www.facebook.com/');


@changangus since we moved the logic to format the ariaLabel prop to this component, I think it's appropriate to test that formatting here. I suggest that we split the current renders links given an href and text in the correct order test into two tests:

renders multiple social links in the correct order: use the current test data with 2 social links. the test will assert that screen.getAllByRole('link') has a length of 2, and that the text content of each link is correct. No need to test other link attributes in this test.

renders a social link: the test data will include a single social link, the test will assert that the link contains the href, text, and formatted aria label.

jonoliver · 2021-12-10T16:47:59Z

Hey Angus, there are just a few more minor spacing issues I noticed. The padding on the the left of the header elements and what appear to be additional columns in the grid layout cause a few minor differences across sections. These issues are mostly visible on large screens. I think that addressing these will bring all sections, other than apprentice qualities, into alignment.

Remove the left padding on this element, as well as the logo. This will align the left side with the sections below.

Remove the additional spacing on the right of the grid in the hero:

Remove the additional spacing on the right of the grid in the CTA:

- Hero cmp - remove padding on logo and text container - remove 2 empty cols from hero art grid - Apprentice cmp - article to div - h2 to h3 - pass name prop to SocialLinks for link aria labels - update test to reflect new alt text for images - Apprentice Status cmp - Add visually hidden span that will act as label for employee status - Link cmp - Use ariaLabel prop to create dynamic aria labels for a tag - update tests - Previous Apprentice cmp - article to section - section to div - add z-index to scss module (padding was covering current apprentice links) - Previous Apprentice Group cmp - p to h3 - add aria label to label apprentice group version - SocialLinks cmp - take passed in name prop and pass it into Link cmp - update tests - Cta cmp - remove 2 empty cols from cta art grid - Home Page - move hero outside of main tag - Updated snapshot FSA21V2-163

jonoliver

Thanks for the updates Angus!

changangus requested a review from jonoliver December 6, 2021 18:21

jonoliver reviewed Dec 7, 2021

View reviewed changes

catheraaine reviewed Dec 8, 2021

View reviewed changes

changangus force-pushed the refactor--semantic-html branch 3 times, most recently from e3ca1a8 to c90ffd0 Compare December 9, 2021 17:03

jonoliver reviewed Dec 9, 2021

View reviewed changes

changangus requested a review from jonoliver December 10, 2021 13:28

changangus force-pushed the refactor--semantic-html branch from c90ffd0 to 8075c09 Compare December 10, 2021 13:29

changangus force-pushed the refactor--semantic-html branch from 8075c09 to cd9207e Compare December 10, 2021 17:10

changangus force-pushed the refactor--semantic-html branch from cd9207e to 6d134ee Compare December 10, 2021 17:14

jonoliver approved these changes Dec 10, 2021

View reviewed changes

auaruss merged commit 6d134ee into main Dec 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Semantic HTML Audit#32

feat: Semantic HTML Audit#32
auaruss merged 1 commit intomainfrom
refactor--semantic-html

changangus commented Dec 6, 2021

Uh oh!

netlify bot commented Dec 6, 2021 •

edited

Loading

Uh oh!

jonoliver Dec 7, 2021

Uh oh!

catheraaine Dec 8, 2021

Uh oh!

jonoliver Dec 7, 2021

Uh oh!

catheraaine Dec 8, 2021

Uh oh!

catheraaine Dec 8, 2021

Uh oh!

catheraaine left a comment

Uh oh!

catheraaine Dec 8, 2021

Uh oh!

catheraaine Dec 8, 2021

Uh oh!

jonoliver Dec 9, 2021

Uh oh!

jonoliver commented Dec 10, 2021

Uh oh!

jonoliver left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

changangus commented Dec 6, 2021

Description

Spec

To Test

Uh oh!

netlify bot commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catheraaine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonoliver commented Dec 10, 2021

Uh oh!

jonoliver left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented Dec 6, 2021 •

edited

Loading