`MobileNav` component for smaller devices #2361

dewanshDT · 2023-08-07T09:38:46Z

This PR is a part of the mobile responsive project

Changes:
added a MobileNav component to be used in smaller devices and, modified the Nav component to use the component in smaller screens.

I have verified that this pull request:

has no linting errors (npm run lint)
has no test errors (npm run test)
is from a uniquely-named feature branch and is up to date with the develop branch.

into develop

# Conflicts: # client/modules/IDE/components/Header/Toolbar.jsx

into develop

lindapaiste

This is coming along great! IMO we should clean it up a bit before merging.

lindapaiste · 2023-08-07T14:56:44Z

client/modules/IDE/components/Header/MobileNav.jsx

+      opacity: 0;
+      transform-origin: top;
+      transition: opacity 100ms;
+      transition: transform 100ms 200ms;


I think if you have two transition lines that the second overrides the first? It should be one line with both transitions in it.

The transitions are really nice though! Good work ⭐

got it! replaced it with a single line, transition: opacity 100ms, transform 100ms 200ms;

transform 100ms 200ms -- that's a delay of 200ms and then 100ms for the scale transform. So the opacity transition will have completed (from 0-100ms) before the transform starts (from 200-300ms). When opening you will only see the transform and when closing you will only see the opacity. Seems like that's probably not what you intended? Maybe should be 100ms for both with no delay? If we want different transitions for opening vs closing we could use react-transition-group.

client/modules/IDE/components/Header/MobileNav.jsx

lindapaiste · 2023-08-07T15:07:08Z

client/modules/IDE/components/Header/MobileNav.jsx

+      default:
+        return project.name;
+    }
+  }


I don’t love this switch based on location. It feels a bit fragile, like if we were to add a new page or change a path or something we would have to change it here as well. Perhaps the alternative is that each page which includes the Nav specifies a mobileTitle prop which we pass down. Like <Nav layout=“dashboard” mobileTitle={t(‘Login’)} />.

yeah that's better, will update it in the future for sure, added a TODO

client/modules/IDE/components/Header/MobileNav.jsx

lindapaiste · 2023-08-07T15:23:41Z

client/modules/IDE/pages/IDEView.jsx

-          <Nav />
+          <Header
+            syncFileContent={this.syncFileContent}
+            key={this.props.project.id}


I wonder if this key prop actually does anything? I see it’s part of the existing code. It doesn’t hurt to leave it but I strongly suspect that it’s useless.

should I remove it?

lindapaiste · 2023-08-07T15:25:33Z

client/theme.js


    Button: {
      primary: {
        default: {
-          foreground: colors.black,
+          foreground: grays.mediumDark,


This change will have implications on all button text colors throughout the app. Is that what we want? @shujuuu Probably yes but I want to double check.

yeah this is should be discussed

lindapaiste · 2023-08-07T15:30:58Z

client/theme.js

+      },
+      secondary: {
+        foreground: colors.black,
+        background: grays.mediumLight


If secondary is its own complete object then does it need a border color?

If the only difference between primary and secondary (and potential tertiary) is the background color and the rest is the same then maybe we structure it a bit differently.

in mobile Panel we don't have borders, I'll check though

client/modules/IDE/components/Header/MobileNav.jsx

lindapaiste · 2023-08-10T01:43:17Z

It seems like the "Find" command isn't working. It's very strange because "Tidy Code" does work. I confirmed that the Editor showFind() method is getting called. It's the internal codemirror part (this._cm.execCommand('findPersistent');) which isn't doing anything.

This could be a real rabbit hole so I will look into it. It seems like the modal is appearing but somehow it's inside of the nav bar and not visible.

Edit: turns out it's just a z-index/positioning issue. Let's add a media query to the _editor.scss file to override the top property of the .CodeMirror-dialog at the same breakpoint where the MobileNav appears. Maybe ~100px down from the top? We need to work on the dialog itself but that's a future problem. It will be usable on tablet sizes if we fix the top position.

client/modules/IDE/components/Header/MobileNav.jsx

lindapaiste · 2023-08-10T17:30:44Z

client/modules/IDE/components/Header/MobileNav.jsx

+          <button onClick={newSketch}>{t('Nav.File.New')}</button>
+        </li>
+        <li>
+          <button onClick={() => saveSketch(cmRef.current)}>


On the desktop nav we have some conditions to hide the "Save" button if the user is not logged in, so we should probably do that here.

Although the usability for clicking it when logged out is pretty decent, it opens a modal telling me to log in.

I like that we're showing the modal to log in instead of hiding the save button when the user is not logged in, what do you think?

Yeah I kind of like it too 🤷 Let's keep it for now but I don't like having different logic on mobile vs desktop. When you get a chance maybe you can open an issue for discussion on how we should handle this. Like whether we should show the "Save" option on desktop and have it open the modal.

We could also def make that modal nicer. Emphasize the "Log In" action (maybe a big button?) and use a friendlier title than "Error".

yeah I agree In the future we should make it nicer by adding a welcoming message with a login button to continue

lindapaiste · 2023-08-10T18:19:51Z

A couple minor style things. Probably to save for later.

The box-shadow seems like a lot. Do we want just a border-bottom?
The account icon could be a little bit smaller.
The top of the menu should probably be aligned with the bottom of the nav bar instead of overlapping it.
We should think about how the user gets to the bottom of the menu when on a short screen (a phone in landscape mode). Right now it will scroll the whole page down. We may want a max-height on the menu of calc(100vh - barHeightpx) and an overflow: auto so that the menu can scroll independent of the page.
If it's easy, we should hide the account menu when on the login page because clicking it won't do anything.

Use core Nav logic in MobileNav.

into dewanshmobile/mobilenav

…T/p5.js-web-editor into dewanshmobile/mobilenav

dewanshDT · 2023-08-12T03:52:10Z

@lindapaiste, I just added an overlay to select language, please have a look, it also indicates the selected language.

# Conflicts: # client/modules/IDE/components/Header/Toolbar.jsx # client/modules/IDE/components/Header/Toolbar.unit.test.jsx

into dewanshmobile/mobilenav

….js-web-editor into dewanshmobile/mobilenav

dewanshDT · 2023-08-14T01:54:15Z

hey @lindapaiste, added the use of ProjectName, please check it out.

client/modules/IDE/components/Header/MobileNav.jsx

client/modules/IDE/components/Header/Toolbar.jsx

client/modules/IDE/components/Header/MobileNav.jsx

lindapaiste · 2023-08-15T15:51:21Z

hey @lindapaiste, added the use of ProjectName, please check it out.

Looking just like the mockups!

lindapaiste · 2023-08-15T17:20:30Z

client/modules/IDE/components/Header/MobileNav.jsx

+                    className={classNames({
+                      'current-language': label === languageKeyToLabel(language)
+                    })}
+                    aria-label={label}


I need to read up on ARIA best practices because I don't know if it's necessary to have an aria-label when it's the same as the text content of the button. But we'll leave it for now.

lindapaiste

You did it! 🥳 🎊 We've fixed all of the major usability issues and I think it's ready to go! This will be such a huge improvement for our mobile users. Next up...the IDE screen.

lindapaiste · 2023-08-15T19:11:42Z

@raclim We've been through a lot of revisions on this PR and we've gotten it to a point where it makes a lot of things better and doesn't make anything worse. There is still a lot to be improved, but it's ready to release!

raclim · 2023-08-16T14:19:25Z

This is super amazing!!! It looks really awesome so far and I think it'll definitely gain a lot of traction from users over the next few weeks, especially as students start going back to school! Thanks again to you both for all of your work! :)

I'll try to merge in #2306, #2052, and #2362 as well before starting the process for the next release, and will give an update when that's happening. Please let me know if you both feel otherwise or would prefer to prioritize any others!

lindapaiste and others added 13 commits June 10, 2023 14:34

use EditableInput in Toolbar

fc6496c

Merge branch 'develop' into fix/toolbar-title

8277cb8

Merge branch 'develop' into fix/toolbar-title

a51d2c6

Merge branch 'develop' into fix/toolbar-title

dafc74d

Merge branch 'develop' of https://github.com/processing/p5.js-web-editor

d6f831d

into develop

Merge remote-tracking branch 'upstream/develop' into fix/toolbar-title

002945e

# Conflicts: # client/modules/IDE/components/Header/Toolbar.jsx

Merge branch 'develop' of https://github.com/processing/p5.js-web-editor

cff161c

into develop

initial MobileNav component

62c710b

account and editor icon

07fd1a9

useHistory

122c5f2

updated the theme for mobile panel

a154a87

updated the Nav component to use the MobileNav component

dc4e9d4

the header and the props

a02694a

dewanshDT added the Area: Mobile label Aug 7, 2023

dewanshDT self-assigned this Aug 7, 2023

dewanshDT requested a review from lindapaiste August 7, 2023 09:39

lindapaiste suggested changes Aug 7, 2023

View reviewed changes

dewanshDT added 7 commits August 8, 2023 00:01

snapshots

694e887

transition override

6ef16e5

memoized the title

33d1223

replaced all navigate with link

2c43627

keyboardShortcuts

e0586a4

cleanup

6c186ef

title translations

60a02f1

dewanshDT requested a review from lindapaiste August 10, 2023 00:34

lindapaiste reviewed Aug 10, 2023

View reviewed changes

client/modules/IDE/components/Header/MobileNav.jsx Outdated Show resolved Hide resolved

lindapaiste reviewed Aug 10, 2023

View reviewed changes

client/modules/IDE/components/Header/MobileNav.jsx Outdated Show resolved Hide resolved

lindapaiste reviewed Aug 10, 2023

View reviewed changes

dewanshDT added 2 commits August 12, 2023 00:45

Merge pull request #3 from lindapaiste/fix/mobilenav-accessibility

2b7596d

Use core Nav logic in MobileNav.

Merge branch 'develop' into dewanshmobile/mobilenav

58e819c

dewanshDT requested a review from lindapaiste August 11, 2023 19:29

dewanshDT added 3 commits August 12, 2023 07:35

Merge branch 'develop' of https://github.com/processing/p5.js-web-editor

3c0ca14

into dewanshmobile/mobilenav

Merge branch 'dewanshmobile/mobilenav' of https://github.com/dewanshD…

9420858

…T/p5.js-web-editor into dewanshmobile/mobilenav

language overlay

77db01e

dewanshDT and others added 6 commits August 12, 2023 09:26

selected language indicator

1088417

Merge remote-tracking branch 'upstream/develop' into fix/toolbar-title

aa3b454

# Conflicts: # client/modules/IDE/components/Header/Toolbar.jsx # client/modules/IDE/components/Header/Toolbar.unit.test.jsx

move ProjectName to a separate file

a49f834

Merge branch 'develop' of https://github.com/processing/p5.js-web-editor

5547591

into dewanshmobile/mobilenav

Merge branch 'fix/toolbar-title' of https://github.com/lindapaiste/p5…

77fd047

….js-web-editor into dewanshmobile/mobilenav

use ProjectName

e8d7890

Merge branch 'develop' into dewanshmobile/mobilenav

334a6f5

lindapaiste suggested changes Aug 15, 2023

View reviewed changes

dewanshDT added 4 commits August 15, 2023 21:30

position

f916303

overflow and lang

c9179e7

dispatch

bd1454e

margin

584de34

lindapaiste reviewed Aug 15, 2023

View reviewed changes

dewanshDT added 3 commits August 15, 2023 23:47

find fix

be46e37

breakpoints and alignment

2a8d3e2

snapshots

cef29cf

lindapaiste approved these changes Aug 15, 2023

View reviewed changes

lindapaiste merged commit f040067 into processing:develop Aug 15, 2023

lindapaiste mentioned this pull request Sep 12, 2023

When the username is too big, the screen pixels are scattered on small screen #2427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`MobileNav` component for smaller devices #2361

`MobileNav` component for smaller devices #2361

dewanshDT commented Aug 7, 2023 •

edited

Loading

lindapaiste left a comment

lindapaiste Aug 7, 2023

dewanshDT Aug 9, 2023

lindapaiste Aug 10, 2023

lindapaiste Aug 7, 2023

dewanshDT Aug 9, 2023

lindapaiste Aug 7, 2023

dewanshDT Aug 10, 2023

lindapaiste Aug 7, 2023

dewanshDT Aug 10, 2023

lindapaiste Aug 7, 2023

dewanshDT Aug 10, 2023

lindapaiste commented Aug 10, 2023 •

edited

Loading

lindapaiste Aug 10, 2023

dewanshDT Aug 11, 2023

lindapaiste Aug 11, 2023

dewanshDT Aug 11, 2023

lindapaiste commented Aug 10, 2023

dewanshDT commented Aug 12, 2023 •

edited

Loading

dewanshDT commented Aug 14, 2023

lindapaiste commented Aug 15, 2023

lindapaiste Aug 15, 2023

lindapaiste left a comment

lindapaiste commented Aug 15, 2023

raclim commented Aug 16, 2023

MobileNav component for smaller devices #2361

MobileNav component for smaller devices #2361

Conversation

dewanshDT commented Aug 7, 2023 • edited Loading

lindapaiste 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

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

lindapaiste commented Aug 10, 2023 • 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

lindapaiste commented Aug 10, 2023

dewanshDT commented Aug 12, 2023 • edited Loading

dewanshDT commented Aug 14, 2023

lindapaiste commented Aug 15, 2023

Choose a reason for hiding this comment

lindapaiste left a comment

Choose a reason for hiding this comment

lindapaiste commented Aug 15, 2023

raclim commented Aug 16, 2023

`MobileNav` component for smaller devices #2361

`MobileNav` component for smaller devices #2361

dewanshDT commented Aug 7, 2023 •

edited

Loading

lindapaiste commented Aug 10, 2023 •

edited

Loading

dewanshDT commented Aug 12, 2023 •

edited

Loading