MarkdownEditor polish #3667
MarkdownEditor polish #3667
Conversation
🦋 Changeset detectedLatest commit: ca78962 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Uh oh! @six7, the image you shared is missing helpful alt text. Check your pull request body. Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
size-limit report 📦
|
| aria-live="polite" | ||
| variant="danger" | ||
| full | ||
| sx={{fontSize: 1, borderBottomLeftRadius: 2, borderBottomRightRadius: 2, px: 2, py: 2}} |
There was a problem hiding this comment.
Do we override Flash/Banner styles in PVC too?
| {label} | ||
| </Button> | ||
| )} | ||
| <TabNav aria-label="Main"> |
There was a problem hiding this comment.
@ichelsea - is this a safe usage of aria-label? Or do we need the text in a visually hidden element that we point to using aria-labelledby?
There was a problem hiding this comment.
I'm not well versed in .tsx, is this ultimately a button? Where is it located/what does it do?
There was a problem hiding this comment.
I took this from https://primer.style/react/TabNav which mentions Attention: Make sure to properly label your TabNav with an aria-label to provide context about the type of navigation contained in TabNav.
Thinking about it, would View mode be a more descriptive label than Main ? I assume a screen reader user would benefit from understanding View mode: Write or View mode: Preview?
There was a problem hiding this comment.
@ichelsea this is used in the MarkdownEditor for the TabNav for Write and Preview.
There was a problem hiding this comment.
Ah, gotcha! So our <TabNav> is essentially a role="tablist" under the hood? We do want it to have an accessible name, so either aria-label or aria-labelledby works here.
"View mode" is definitely a step in the right direction vs. "main", especially because "main" is used in main landmark identification. Something about it still feels like it needs adjustment, but I don't have a better suggestion at the moment.
There was a problem hiding this comment.
I updated the text string to use View mode now and will merge in given the existing approvals. Given the discussion around the Rails CommentBox I do anticipate some changes down the road for the Rails component as well as this one as we want to make sure it stays aligned. The React component is already being used in the Memex Sidesheet (and the new Issues and PR experiences) if you do want to check it out once it's merged 👍
There was a problem hiding this comment.
I'd like @simurai to take a look too. I believe he spent a lot of time with the markdown editor component design.
This looks pretty close to the Rails version. Which I think just GAed. Not sure who initially created this React version, maybe Memex team? But maybe good to keep both versions somewhat aligned. 👍
There'd still be work left to fully align, namely the way we upload (moving the attach button to the toolbar). I didn't work on that as I think that'd grow this PR in size even further.
There was feedback that the uploading in Rails is too subtle. It just shows some text inline. So also having a Banner at the bottom might be a good addition.
This reverts commit 791c983.
This PR aligns MarkdownEditor with the Rails version to at least visually come closer to the experience. It features a Write / Preview TabNav experience, a visually subdued toolbar, and an action area that lies visually beneath the markdown editor.
This relates to #3604 and could just be included in the ongoing work there. However, it was noted that the work for this is not planned right now, so it'd be better to ship these updates so we can later consolidate.
The toolbar on smaller screens still wraps like the previous version, so no work was done to introduce an ActionBar. This should be easy as this is being worked on internally right now, and once #3604 is complete we should be able to introduce the ActionBar there as well for proper responsive behavior. I did remove the
Markdown is supportedhint, as we don't have that in the Rails version either. Happy to revert that, but I think we should align components.There'd still be work left to fully align, namely the way we upload (moving the attach button to the toolbar). I didn't work on that as I think that'd grow this PR in size even further.
Screenshots
Merge checklist