Skip to content

Add main as a wrapper for story content#2988

Merged
snowystinger merged 18 commits intoadobe:mainfrom
lishichengyan:issue-1268-add-main-as-wrapper-for-story-content
Apr 7, 2022
Merged

Add main as a wrapper for story content#2988
snowystinger merged 18 commits intoadobe:mainfrom
lishichengyan:issue-1268-add-main-as-wrapper-for-story-content

Conversation

@lishichengyan
Copy link
Copy Markdown
Contributor

Closes
#1268
This seems to be a real quick fix -- simply adding a main wrapper in .storybook/custom-addons/provider/index.js would do the trick.

Before:
Screen Shot 2022-03-28 at 2 55 32 PM

After:
Screen Shot 2022-03-28 at 2 56 34 PM

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. add the aXe devtools to your chrome extension
  2. choose a story arbitrarily and open it in a new tab
  3. run aXe against the entire page, check if the warning is still there

🧢 Your Project:

Copy link
Copy Markdown
Collaborator

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

Did you confirm that opening this in another tab via the button Danni mentioned did or didn't work for aXe and landmark?

@lishichengyan
Copy link
Copy Markdown
Contributor Author

Did you confirm that opening this in another tab via the button Danni mentioned did or didn't work for aXe and landmark?

Yes, i checked the "no main landmark" error is gone (see the second screenshot above)

@LFDanLu
Copy link
Copy Markdown
Member

LFDanLu commented Mar 28, 2022

GET_BUILD

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

LFDanLu
LFDanLu previously approved these changes Mar 28, 2022
Copy link
Copy Markdown
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

LGTM, will have to look at updating the landmark stories since there will be multiple main landmarks then but that is out of scope here IMO

Comment on lines +57 to +59
<main>
{storyReady && props.children}
</main>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should main also wrap the div that precedes the story content?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey Michael, thanks for reviewing! Yeah we can do that, but as long as we provide a main landmark the "no main landmark" error should be gone. The question is whether or not should we consider the "note" button as part of "main" content. 🤔

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's fine for the note to be part of the main content. If not, axe might throw an error or warning when the note is present, because all content within the page is not contained within landmarks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR. Thank!

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.

7 participants