Mobile Sketch Preview Screen #1467
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,24 @@ | ||
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import styled from 'styled-components'; | ||
| import Button from '../../common/Button'; | ||
|
|
||
| const ButtonWrapper = styled(Button)` | ||
| width: 3rem; | ||
| > svg { | ||
| width: 100%; | ||
| height: 100%; | ||
| } | ||
| `; | ||
|
|
||
| const IconButton = props => (<ButtonWrapper | ||
| iconBefore={props.children} | ||
| kind={Button.kinds.inline} | ||
| {...{ ...props, children: null }} | ||
|
There was a problem hiding this comment. Do you need to put There was a problem hiding this comment. Yes, it's null if I don't pass anything, but in this case I wanted to pass There was a problem hiding this comment. I introduced an |
||
| />); | ||
|
|
||
| IconButton.propTypes = { | ||
| children: PropTypes.element.isRequired | ||
| }; | ||
|
|
||
| export default IconButton; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import styled from 'styled-components'; | ||
|
|
@@ -23,59 +22,56 @@ import { getHTMLFile } from '../reducers/files'; | |
| import Editor from '../components/Editor'; | ||
| import { ExitIcon } from '../../../common/icons'; | ||
|
|
||
| import { PreferencesIcon, PlayIcon } from '../../../common/icons'; | ||
|
|
||
| import IconButton from '../../../components/mobile/IconButton'; | ||
| import Header from '../../../components/mobile/Header'; | ||
| import Screen from '../../../components/mobile/MobileScreen'; | ||
| import Footer from '../../../components/mobile/Footer'; | ||
| import IDEWrapper from '../../../components/mobile/IDEWrapper'; | ||
|
|
||
| const IconContainer = styled.div` | ||
| marginLeft: 2rem; | ||
| display: flex; | ||
| `; | ||
|
|
||
| const TitleContainer = styled.div` | ||
|
|
||
| `; | ||
|
|
||
| const isUserOwner = ({ project, user }) => (project.owner && project.owner.id === user.id); | ||
|
|
||
| const IDEViewMobile = (props) => { | ||
| const { | ||
| preferences, ide, editorAccessibility, project, updateLintMessage, clearLintMessage, | ||
| selectedFile, updateFileContent, files, | ||
| closeEditorOptions, showEditorOptions, showKeyboardShortcutModal, setUnsavedChanges, | ||
| startRefreshSketch, stopSketch, expandSidebar, collapseSidebar, clearConsole, console, | ||
| showRuntimeErrorWarning, hideRuntimeErrorWarning, startSketch | ||
| } = props; | ||
|
|
||
| const [tmController, setTmController] = useState(null); // eslint-disable-line | ||
| const [overlay, setOverlay] = useState(null); // eslint-disable-line | ||
|
|
||
| return ( | ||
| <Screen> | ||
| <Header> | ||
| <IconButton to="/mobile" aria-label="Return to original editor"> | ||
| <ExitIcon viewBox="0 0 16 16" /> | ||
|
There was a problem hiding this comment. Are you using viewbox here to change the size of the icon? I would prefer if you explicitly set the width and height, and maybe this needs to be added to There was a problem hiding this comment. I'll remove the |
||
| </IconButton> | ||
| <div style={{ marginLeft: '1rem' }}> | ||
|
There was a problem hiding this comment. if possible i would prefer to not have inline styles! There was a problem hiding this comment. The next PR fixes this by improving the structure of the |
||
| <h2>{project.name}</h2> | ||
| <h3>{selectedFile.name}</h3> | ||
| </div> | ||
|
|
||
| <IconContainer> | ||
| <IconButton onClick={() => setOverlay('preferences')}> | ||
| <PreferencesIcon focusable="false" aria-hidden="true" /> | ||
| </IconButton> | ||
| <IconButton to="/mobile/preview" onClick={() => { startSketch(); }}> | ||
| <PlayIcon viewBox="-1 -1 7 7" focusable="false" aria-hidden="true" /> | ||
| </IconButton> | ||
| </IconContainer> | ||
| </Header> | ||
|
|
||
| <IDEWrapper> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,13 @@ export default { | |
| Icon: { | ||
| default: grays.middleGray, | ||
| hover: grays.darker | ||
| }, | ||
| Panel: { | ||
|
There was a problem hiding this comment. These are same colours as the buttons, so we'll have a problem if someone later puts a Button on top of a Panel as they'll be the same. Shall we take the colours from the console header component? Cassie made accessibility colour changes a a while back to ensure the contrast worked across all the themes. So:
The colours are here. I know it's a bit of a pain to do this, but it'll really help with the move to styled-components and away from SCSS and helping to make the design more consistent. There was a problem hiding this comment. True, but these are the colors that Panels use on the mobile UI (on Headers and Footers at least), so I'm not sure what to do. I wonder if they even mean the same thing on both UIs. I think I'll create a There was a problem hiding this comment. If possible, it would be great to have the same set of colours work for mobile and the legacy UI. My feeling is branching the theme into Panel.mobile and Panel.notMobile creates more code for us to maintain. Your right that maybe this isn't a reusable panel colour, so if it's not possible to find common colours, then my preference is that we name the variable something other than Panel. @catarak, what do you think? There was a problem hiding this comment. I investigated this a bit, and It's an important decision that goes beyond the scope of the PR, so I'll create an issue and we can discuss it there! There was a problem hiding this comment. Ok, good idea. So shall we call this MobilePanel or something for now and we can get this merged in? There was a problem hiding this comment. I agree with @andrewn, that it would be helpful if possible to not have mobile specific colors. @ghalestrilo and I talked this over and thought it would make sense to open a new issue/PR with respect to the panel and button color contrasts, see #1484. There was a problem hiding this comment. Nice! I'll update this :) |
||
| default: { | ||
| foreground: colors.black, | ||
| background: grays.light, | ||
| border: grays.middleLight, | ||
| }, | ||
| } | ||
| }, | ||
| [Theme.dark]: { | ||
|
|
@@ -120,6 +127,13 @@ export default { | |
| Icon: { | ||
| default: grays.middleLight, | ||
| hover: grays.lightest | ||
| }, | ||
| Panel: { | ||
| default: { | ||
| foreground: grays.light, | ||
| background: grays.dark, | ||
| border: grays.middleDark, | ||
| }, | ||
| } | ||
| }, | ||
| [Theme.contrast]: { | ||
|
|
@@ -152,6 +166,13 @@ export default { | |
| Icon: { | ||
| default: grays.mediumLight, | ||
| hover: colors.yellow | ||
| }, | ||
| Panel: { | ||
| default: { | ||
| foreground: grays.light, | ||
| background: grays.dark, | ||
| border: grays.middleDark, | ||
| }, | ||
| } | ||
| }, | ||
| }; | ||
Uh oh!
There was an error while loading. Please reload this page.