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

Add second section for modals and move away from absolute positionning #1715

Merged
merged 16 commits into from
Nov 11, 2021

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Nov 8, 2021

closes #1664
I tested some modal such as the sharing explainer.

todo,test the following:

  • Shared transfer
  • Sharing explainer
  • Team modal (fake door)
  • Folder creation
  • Create or edit shared folder
  • File Info
  • Move File
  • Report File
  • Share Modal
  • Upload File
  • Mobile folder/file edit
  • forget browser

To test the 2nd section you can add the prop subModal={<div>Some section here</div>} to any CustomModal element, I did it in the following video for the Shared modal creation.

Modal.mp4

@render
Copy link

render bot commented Nov 8, 2021

@render
Copy link

render bot commented Nov 8, 2021

@render
Copy link

render bot commented Nov 8, 2021

@github-actions github-actions bot added the Type: Bug Fix Added to PRs if they are addressing a bug label Nov 8, 2021
Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

The Delete confirmation needs padding and radius:
image
Previously:
image

@Tbaut
Copy link
Collaborator Author

Tbaut commented Nov 9, 2021

Good catch, the Dialog was also using Modal. It should be fixed now. I checked a couple other modals, and they looked fine, but a proper go around is prob needed again.

@asnaith
Copy link
Contributor

asnaith commented Nov 10, 2021

I inspected all the modals on the list on the desktop without issue but noticed multiple problems on mobile (iOS)

New issues:

  • For modals that have a cancel button outside of the modal, when the keyboard is invoked it covers the cancel button (on production the cancel button is adjusted so that it is displayed above the keyboard).

  • "Create or edit shared folder" modal (mobile): After invoking a “Folder name is already in use” error the input bar needs to be tapped a few times before it responds and can be edited again.

  • "Create or edit shared folder" modal (mobile): I managed to cause an issue where the cursor was appearing on the wrong part of the screen and not where I was typing. I couldn't nail down reproduction steps but caught it on video.

create.share.cursor.positioning.MP4
  • "Report File" modal (mobile): The decryption key is being cut off

  • Rename file modal (mobile): The modal is fixed in the middle but the background is scrollable (this happens on all but it is more noticeable on smaller modals like this).
upload.file.scrolling.MP4
  • "Manage Access" modal (mobile): Only 2 / 3 characters of the share link url are shown the rest of the string is truncated due to lack of room (side note - looks like link sharing is not available in prod for mobile view so I guess we all missed that!).

  • "Forget browser" modal (mobile): The cancel button is within the modal but the top corners are rounded and the bottom is straight.

Issues that also occur in prod

Some issues are already present in prod so they are probably out of the scope here but I wanted to mention them first and then we can decide whether to create separate issues:

  • Some modals have the cancel / close button inside the actual modal. Others it is a secondary bar fixed to the bottom of the screen. Lots of inconsistencies.

  • Modals that have a cancel button outside the modal have rounded corners on the top of the modal but straight edges on the bottom. This is the same in prod but now the distance between modal and cancel bar is larger the straight edges look more like a bug than intentional design.

  • Rotating the phone to landscape and back to portrait whilst a modal is in view causes the zoom level to get messed up and requires the user to pinch to zoom back out.

  • "Share" modal (mobile): The placeholder text is very close to the edges of the input box, some padding would make this look nicer.

  • Tapping a dropdown or text input on a modal adjusts the zoom level of the screen and requires the user to manually zoom out to return to normal.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Nov 10, 2021

Thanks for the thorough review. Many issues here are actually also happening in prod. I'll try to fix some, but I don't want to keep this PR open forever and try to fix them all here. The potential to add some more breaking changes is high and I value small and to the point PRs rather than big catch-all.

For modals that have a cancel button outside of the modal, when the keyboard is invoked it covers the cancel button (on production the cancel button is adjusted so that it is displayed above the keyboard).

But you can scroll to it right? I wonder if showing a cancel button all the time above the keyboard is the way to go. it takes quite some display real estate.

"Create or edit shared folder" modal (mobile): After invoking a “Folder name is already in use” error the input bar needs to be tapped a few times before it responds and can be edited again.

Can you please confirm this doesn't happen in prob?

Rename file modal (mobile): The modal is fixed in the middle but the background is scrollable

That's also in prod. Applying a overflow: hidden to the body should fix it, I'll consider it (it's not that trivial and could break yet other things).
edit: added it, I think it's doing a pretty good job.

"Report File" modal (mobile): The decryption key is being cut off

This must happen in prod too. We have an issue related that could handle this as well #1720 and #1713

Issues that also occur in prod

Thanks for finding all those. Would you mind creating issues for these. This PR breaks many things already for me to take care of everything here that's already broken :)

@asnaith
Copy link
Contributor

asnaith commented Nov 11, 2021

For modals that have a cancel button outside of the modal, when the keyboard is invoked it covers the cancel button (on production the cancel button is adjusted so that it is displayed above the keyboard).

But you can scroll to it right? I wonder if showing a cancel button all the time above the keyboard is the way to go. it takes quite some display real estate.

Yeah, you can still scroll to it. I agree that we are already limited on available space and showing it limits it even more. I just wanted to mention it as it differed from prod behaviour.

"Create or edit shared folder" modal (mobile): After invoking a “Folder name is already in use” error the input bar needs to be tapped a few times before it responds and can be edited again.

Can you please confirm this doesn't happen in prob?

Yep, I wasn't able to reproduce this on production but I also can't reproduce now either. Sorry for the red herring, might have been an issue between localhost and my phone

Rename file modal (mobile): The modal is fixed in the middle but the background is scrollable

That's also in prod. Applying a overflow: hidden to the body should fix it, I'll consider it (it's not that trivial and could break yet other things). edit: added it, I think it's doing a pretty good job.

Nice, I'll check it out.

"Report File" modal (mobile): The decryption key is being cut off

This must happen in prod too. We have an issue related that could handle this as well #1720 and #1713

Ah yes you're right it's in prod. The issues you mentioned are for the "File Info" modal though (but similar fix probably). I created a separate issue for the "Report File" modal #1726.

Issues that also occur in prod

Thanks for finding all those. Would you mind creating issues for these. This PR breaks many things already for me to take care of everything here that's already broken :)

Yep pre-existing ones are now separate issues #1727, #1728, #1729

Apologies for the myriad of things discussed at once in this PR. I just had to read it all back for my own sake/sanity 😅.

TL;DR I think everything discussed here that is fixable we will address in another issue now (due to them being pre-existing). I can't recreate the weirdness with the mispositioned cursor or frozen input so they could have been edge cases that exist in production too. I'll continue to see if I can recreate those issues but they shouldn't hold up this change imo.

@Tbaut Tbaut requested review from FSM1 and RyRy79261 November 11, 2021 10:46
@FSM1 FSM1 merged commit 2190e90 into dev Nov 11, 2021
@FSM1 FSM1 deleted the fix/tbaut-modal-1664 branch November 11, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix Added to PRs if they are addressing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modals cut off at the top (mobile)
5 participants