Skip to content
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

Hook up sidebar tabs to mobile view & add icons to sidebar tabs #2242

Merged
merged 4 commits into from
Oct 19, 2022

Conversation

ianyong
Copy link
Member

@ianyong ianyong commented Oct 17, 2022

Description

Hook up sidebar tabs to the mobile view.
image
Sidebar tabs are only displayed in the mobile view (as normal side content tabs) if they have an id specified:

// Convert sidebar tabs with a side content tab ID into side content tabs.
const sideBarTabs: SideContentTab[] = props.sideBarProps.tabs.filter(tab => tab.id !== undefined);

Also add icons to sidebar tabs (since side content tabs require them anyway).
image

In the mobile view, long presses are converted into the equivalent of right clicks on the desktop view by most browsers.

Part of #2176.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

Use the developer tools to force the mobile view and verify that the file system view has all of the functionality of the desktop view as described in #2236.

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately "hold-to-right-click" doesn't seem to be working on iOS, so I am unable to create any new files when testing on Apple devices. At the very least, an "add file" button needs to be added so that I can proceed to test if "hold-to-right-click" still works on files/directories. Note that the "add file" button needs to be added to both mobile and desktop view since there are tablets that use desktop view.

IMG_7773.MOV

^ Tested on Chrome on iPhone

IMG_1846.MP4

^ Tested on Chrome on iPad

Furthermore, when there are no files, the files tab on mobile view is blank. Showing some introductory text may be beneficial for students who may be confused by the blank screen.

@ianyong
Copy link
Member Author

ianyong commented Oct 18, 2022

Thanks for testing!

Unfortunately "hold-to-right-click" doesn't seem to be working on iOS, so I am unable to create any new files when testing on Apple devices.

Since all browsers on iOS are actually running WebKit under the hood, this seems to be a Safari issue.

Firefox on Android works:
https://user-images.githubusercontent.com/5585517/196336375-98ab6e9e-d4b5-4f05-9531-d418520e1575.mp4

It would seem to me that this issue of long pressing an item resulting in some other item being selected is an issue with the browser and not the code. I have no idea what can possibly be done to fix this at the Source Academy level nor do I have the means of debugging this issue.

At the very least, an "add file" button needs to be added so that I can proceed to test if "hold-to-right-click" still works on files/directories. Note that the "add file" button needs to be added to both mobile and desktop view since there are tablets that use desktop view.

Why would it work on files/directories but not on the empty space? The context menu component that is used is the same, and the problem of long presses selecting something else is a browser bug.

Also, this is not an acceptable compromise. As more features are added, we need to be aware that the context menu might look something like this in the future:
image

How do you propose we add a button on the UI for every single action that can be performed? There is also the issue whereby actions are performed on a particular file/directory, meaning that we need both a target and an action. When the "add file" button is pressed, where should the file go? We would need to introduce the notion of a working directory somehow.

Furthermore, when there are no files, the files tab on mobile view is blank. Showing some introductory text may be beneficial for students who may be confused by the blank screen.

This is a good point which I have intentionally held off on as discussed in the last point of #2236 (comment) because (1) things will be clearer as we progress and (2) the aim right now is to come up with a minimum viable product.

@zhaojj2209 Unfortunately, I will need your help to debug the long press issue on iOS.

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the lack of clarification, the suggestion to add a "Add file" button was for testing purposes. I've since added a file manually through the code to test, and have confirmed that "hold-to-right-click" doesn't work even when there are files, either.

You're right in that this is probably a browser issue, in which case we will likely need to look for workarounds. We can come back to this issue at a later time; other than this, LGTM.

@ianyong
Copy link
Member Author

ianyong commented Oct 19, 2022

You're right in that this is probably a browser issue, in which case we will likely need to look for workarounds. We can come back to this issue at a later time; other than this, LGTM.

Tracking in #2243.

@ianyong ianyong merged commit e2c737d into add-file-system-view Oct 19, 2022
@ianyong ianyong deleted the hook-up-sidebar-tabs-to-mobile-view branch October 19, 2022 12:56
@martin-henz
Copy link
Member

Thanks for testing!

Unfortunately "hold-to-right-click" doesn't seem to be working on iOS, so I am unable to create any new files when testing on Apple devices.

Since all browsers on iOS are actually running WebKit under the hood, this seems to be a Safari issue.

Firefox on Android works: https://user-images.githubusercontent.com/5585517/196336375-98ab6e9e-d4b5-4f05-9531-d418520e1575.mp4

It would seem to me that this issue of long pressing an item resulting in some other item being selected is an issue with the browser and not the code. I have no idea what can possibly be done to fix this at the Source Academy level nor do I have the means of debugging this issue.

At the very least, an "add file" button needs to be added so that I can proceed to test if "hold-to-right-click" still works on files/directories. Note that the "add file" button needs to be added to both mobile and desktop view since there are tablets that use desktop view.

Why would it work on files/directories but not on the empty space? The context menu component that is used is the same, and the problem of long presses selecting something else is a browser bug.

Also, this is not an acceptable compromise. As more features are added, we need to be aware that the context menu might look something like this in the future: image

How do you propose we add a button on the UI for every single action that can be performed? There is also the issue whereby actions are performed on a particular file/directory, meaning that we need both a target and an action. When the "add file" button is pressed, where should the file go? We would need to introduce the notion of a working directory somehow.

Furthermore, when there are no files, the files tab on mobile view is blank. Showing some introductory text may be beneficial for students who may be confused by the blank screen.

This is a good point which I have intentionally held off on as discussed in the last point of #2236 (comment) because (1) things will be clearer as we progress and (2) the aim right now is to come up with a minimum viable product.

@zhaojj2209 Unfortunately, I will need your help to debug the long press issue on iOS.

Currently, SA does not support Safari, so this is not a major issue. We may look into Safari support in a future project, so you could log the Safari-specific issue in the frontend with a subject prefix "Safari:".

ianyong added a commit that referenced this pull request Dec 4, 2022
* Take in sidebar tabs as prop in MobileWorkspace

* Add icon to sidebar tabs

* Add sidebar tabs to mobile view

* Disable draggable REPL on mobile view when on files tab
ianyong added a commit that referenced this pull request Dec 8, 2022
* Take in sidebar tabs as prop in MobileWorkspace

* Add icon to sidebar tabs

* Add sidebar tabs to mobile view

* Disable draggable REPL on mobile view when on files tab
ianyong added a commit that referenced this pull request Dec 14, 2022
* Take in sidebar tabs as prop in MobileWorkspace

* Add icon to sidebar tabs

* Add sidebar tabs to mobile view

* Disable draggable REPL on mobile view when on files tab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants