-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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!
| 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