-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add Editor component to the Mobile IDE View #1459
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
Conversation
<h2>{project.name}</h2> | ||
<h3>{selectedFile.name}</h3> | ||
</div> | ||
<Icon href="/"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should import CloseIcon
from client/common/icons.jsx
, and wrap it in a <Link>
or <button>
depending on what you want it to do. And then, because this icon has no text but performs an action, it should have the attribute aria-label="close header"
(or whatever the alt text should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do that!
@@ -92,18 +91,14 @@ const IDEViewMobile = (props) => { | |||
return ( | |||
<Screen> | |||
<Header> | |||
<Link to="/" style={{ width: '3rem', marginRight: '1.25rem' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're using styled components, I think it makes sense to not use any inline CSS like this! You also shouldn't need aria-label
or aria-hidden
on the CloseIcon
, as aria-label
should be added to the <Link>
, see https://www.scottohara.me/blog/2019/05/22/contextual-images-svgs-and-a11y.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Just updated this!
c55952b
to
a99d2b8
Compare
This PR mounts the
<Editor />
component on the Mobile IDE View (/mobile
), and places a button for exiting the mobile view.Functionality is only partially implement, as expected: running the code will require a new overlay component.
I have verified that this pull request:
npm run lint
)Fixes #123