Mobile Sketch Preview Screen #1467

ghalestrilo · 2020-06-19T21:12:46Z

Part of the Mobile UI project. This PR adds a (working!) Mobile Sketch Preview screen (/mobile/preview), which renders the canvas with the project files and resizes it to properly fit the mobile view.

How to test:

Run the app locally with the MOBILE_ENABLED flag set to true (MOBILE_ENABLED=true npm start)

You should be able to access /mobile
You should be able to edit code
Clicking the play icon should run the sketch and you to the preview screen screen
You should be able to go back to the code screen, and repeat the process without errors
Clicking the exit icon should take you to the default editor

Run the app without the flag enabled

It should work as usual
/mobile routes should not be accessible

I have verified that this pull request:

has no linting errors (npm run lint)
is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
is descriptively named and links to an issue number, i.e. Fixes #123

andrewn

This is looking really great!

I have some comments and questions about the code. If this has been addressed in your next PR (or will be) then feel free to say that. :-D

client/modules/IDE/components/PreviewFrame.jsx

andrewn · 2020-06-29T11:21:34Z

client/modules/IDE/components/PreviewFrame.jsx

-      this.renderSketch();
-    }
-
+    if (shouldRenderSketch(this.props, prevProps)) this.renderSketch();
    // small bug - if autorefresh is on, and the usr changes files


Is this a bug that still exists? Should this comment be removed?

I think the commend holds, since I merely moved code around. The logic is pretty much the same

client/components/mobile/Header.jsx

client/components/mobile/IconButton.jsx

client/modules/IDE/components/PreviewFrame.jsx

client/modules/IDE/pages/IDEViewMobile.jsx

client/modules/Mobile/MobileSketchView.jsx

andrewn · 2020-06-29T12:08:20Z

client/modules/Mobile/MobileSketchView.jsx

+        </IconLinkWrapper>
+        <div>
+          <h2>{projectName}</h2>
+          <h3><br /></h3>


What's the purpose of this mark-up?

<h3><br /></h3>

It just heightens the header by an h3 height, to keep the h2 tag aligned with the previous screen - couldn't think of a better way to do this...

We might have to use CSS to add padding to the h2? And maybe set the previous screens header to a know height?

Using h3 is this way is not semantic especially as screen-readers allow the user to list and navigate via the headings in the page, which will be empty.

Good point. I'm fixing this on the next PR, and improving the <Header /> component as well

client/routes.jsx

andrewn · 2020-06-29T12:19:22Z

I ran it on my iPhone and it works great! But the preferences and play icons are a bit small compared the the exit icon. They're a bit hard to tap and also a different colour with no press colour change.

andrewn · 2020-06-30T09:54:28Z

client/theme.js

@@ -88,6 +88,13 @@ export default {
    Icon: {
      default: grays.middleGray,
      hover: grays.darker
+    },
+    Panel: {


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:

Panel.default.foreground should be the value of console-header-color
Panel.default.background should be the value of console-header-background-color
Panel.default.border should be the value of ide-border-color

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.

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 Panel: { mobile: {} } attribute and keep these colors there, so the default Panel colors can be set to those of the default UI. Do you think that makes sense?

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?

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!

Ok, good idea.

So shall we call this MobilePanel or something for now and we can get this merged in?

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.

Nice! I'll update this :)

catarak · 2020-06-30T22:00:45Z

client/components/mobile/IconButton.jsx

+const IconButton = props => (<ButtonWrapper
+  iconBefore={props.children}
+  kind={Button.kinds.inline}
+  {...{ ...props, children: null }}


Do you need to put children: null here? Isn't children null here if you just don't put anything in between <ButtonWrapper></ButtonWrapper>?

Yes, it's null if I don't pass anything, but in this case I wanted to pass children into the iconBefore prop, and not into the body of the component.
I thought of a better solution, so I'll change this soon

I introduced an icon prop, now it looks a lot cleaner

client/modules/IDE/components/PreviewFrame.jsx

catarak · 2020-06-30T22:09:11Z

client/modules/IDE/pages/IDEViewMobile.jsx

-        </IconLinkWrapper>
-        <div>
+        <IconButton to="/mobile" aria-label="Return to original editor">
+          <ExitIcon viewBox="0 0 16 16" />


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 client/common/icons.jsx, which could be done in a separate PR.

I'll remove the viewbox prop for now, and find a way to resize the icon in a future PR, to minimize conflicts with the other branch

catarak · 2020-06-30T22:11:43Z

client/modules/IDE/pages/IDEViewMobile.jsx

+        <IconButton to="/mobile" aria-label="Return to original editor">
+          <ExitIcon viewBox="0 0 16 16" />
+        </IconButton>
+        <div style={{ marginLeft: '1rem' }}>


if possible i would prefer to not have inline styles!

The next PR fixes this by improving the structure of the <Header /> component

ghalestrilo added 16 commits June 16, 2020 16:25

🚧 emplace settings and play icons

0d119aa

🚧 add icons to header

bdedc63

🚧 split components into files

0877d39

🚧 add MobileSketchView screen to /mobile/preview

5c80702

🚧 make link to /mobile

49a9fe7

🚧 create alternate content wrapper for proper top padding

e5bbb53

🚧 mount <PreviewFrame /> on /mobile/preview

0633c3b

🚧 correct hook call for getProject

e11756d

🚧 make mobile IDE screen start the sketch

8a067a8

🔀 merge from origin/develop

123c2b0

🚧 mound the <PreviewFrame /> properly on Mobile Preview screen

d00506a

✨ render sketch

a11bc44

♻️ refactor call to PreviewFrame#renderSketch

776c3d2

🧹 clean up routes.js

eb3bc59

🐛 correct projectName propType to string

0e66756

✨ strech preview canvas to full-width on demand

597cb9b

ghalestrilo mentioned this pull request Jun 24, 2020

Mobile Preferences Screen Prototype #1472

Merged

3 tasks

ghalestrilo requested a review from andrewn June 24, 2020 19:23

andrewn reviewed Jun 29, 2020

View reviewed changes

ghalestrilo added 10 commits June 29, 2020 14:30

👌 mark comment on routes.jsx as TODO

7604e27

👌 remove empty useEffect

ec8566f

👌 move icons to common/icons

ebb9525

👌 remove eslint-disable

75bd5a3

👌 remove debugger comment

1eb1bff

👌 remove linter comment

c39211d

👌 remove eslint-disable

78ec304

👌 create Panel color object in theme

9ca0995

👌 use common/Button component for IconButton

7d24c07

👌 use common/Button component for IconButton on screens

8a0b09d

🐛 fix margin on header title

7805acc

andrewn reviewed Jun 30, 2020

View reviewed changes

ghalestrilo mentioned this pull request Jun 30, 2020

Panel color in all themes #1484

Closed

catarak requested changes Jun 30, 2020

View reviewed changes

ghalestrilo added 3 commits July 1, 2020 15:36

👌 rename Panel to MobilePanel on theme.js

a1d6abf

👌 change IconButton structure

3143cc3

👌 improve IconButton implementation

b4c1b86

ghalestrilo requested a review from catarak July 1, 2020 19:44

catarak merged commit ff658c7 into processing:develop Jul 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile Sketch Preview Screen #1467

Mobile Sketch Preview Screen #1467

ghalestrilo commented Jun 19, 2020 •

edited

Loading

andrewn left a comment

andrewn Jun 29, 2020

ghalestrilo Jun 29, 2020

andrewn Jun 29, 2020

ghalestrilo Jun 29, 2020

andrewn Jun 30, 2020

ghalestrilo Jun 30, 2020

andrewn commented Jun 29, 2020 •

edited

Loading

andrewn Jun 30, 2020

ghalestrilo Jun 30, 2020 •

edited

Loading

andrewn Jun 30, 2020

ghalestrilo Jun 30, 2020

andrewn Jun 30, 2020

catarak Jun 30, 2020 •

edited

Loading

ghalestrilo Jul 1, 2020

catarak Jun 30, 2020

ghalestrilo Jul 1, 2020

ghalestrilo Jul 1, 2020

catarak Jun 30, 2020

ghalestrilo Jul 1, 2020

catarak Jun 30, 2020

ghalestrilo Jul 1, 2020

Mobile Sketch Preview Screen #1467

Mobile Sketch Preview Screen #1467

Conversation

ghalestrilo commented Jun 19, 2020 • edited Loading

andrewn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewn commented Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

ghalestrilo Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catarak Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalestrilo commented Jun 19, 2020 •

edited

Loading

andrewn commented Jun 29, 2020 •

edited

Loading

ghalestrilo Jun 30, 2020 •

edited

Loading

catarak Jun 30, 2020 •

edited

Loading