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

File preview updates #1960

Merged
merged 18 commits into from
Feb 23, 2022
Merged

File preview updates #1960

merged 18 commits into from
Feb 23, 2022

Conversation

FSM1
Copy link
Contributor

@FSM1 FSM1 commented Feb 21, 2022

closes #1909


Submission checklist:

Layout

  • Change looks good in the desktop web ui
  • Change looks good in the mobile web ui

Theme

  • Components / elements inspected in light mode
  • Components / elements inspected in dark mode

@render
Copy link

render bot commented Feb 21, 2022

@render
Copy link

render bot commented Feb 21, 2022

@render
Copy link

render bot commented Feb 21, 2022

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Feb 21, 2022
@lgtm-com
Copy link

lgtm-com bot commented Feb 21, 2022

This pull request introduces 1 alert when merging c7f11e4 into f645120 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Feb 21, 2022

This pull request introduces 1 alert when merging c7d3bb8 into f645120 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@FSM1 FSM1 requested review from Tbaut, tanmoyAtb and asnaith February 21, 2022 10:30
@FSM1 FSM1 changed the title Feat/file preview updates 1909 File preview updates Feb 21, 2022
@FSM1 FSM1 marked this pull request as ready for review February 21, 2022 10:32
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I've had some small issues with the "active" state of buttons.
like below on the "go back to overall view" button
image

Or the next button

next.mp4

FSM1 and others added 2 commits February 21, 2022 18:05
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@FSM1
Copy link
Contributor Author

FSM1 commented Feb 22, 2022

I've had some small issues with the "active" state of buttons. like below on the "go back to overall view" button image

Or the next button

next.mp4

Yeah the active state does mess things up, but this is out of the scope of this issue. The problem should be resolved in the button component in my mind.

@Tbaut
Copy link
Collaborator

Tbaut commented Feb 22, 2022

yeah the problem I mentioned are all on the active state of buttons, both the arrow and the "back to overall view"

@tanmoyAtb
Copy link
Contributor

All previews seem to be working great,

Only for html files, I found the zoom buttons on top right appearing behind,
Here's a preview -
Screenshot 2022-02-22 at 11 37 00 PM

@FSM1
Copy link
Contributor Author

FSM1 commented Feb 22, 2022

All previews seem to be working great,

Only for html files, I found the zoom buttons on top right appearing behind, Here's a preview - Screenshot 2022-02-22 at 11 37 00 PM

ahhh whoops, I may have completely forgotten to look at the text file renderer. let me check that out again. will ping you when I have tested that

@FSM1
Copy link
Contributor Author

FSM1 commented Feb 22, 2022

All previews seem to be working great,
Only for html files, I found the zoom buttons on top right appearing behind, Here's a preview - Screenshot 2022-02-22 at 11 37 00 PM

ahhh whoops, I may have completely forgotten to look at the text file renderer. let me check that out again. will ping you when I have tested that

Fixed in the latest commit. Great catch @tanmoyAtb.

@FSM1 FSM1 self-assigned this Feb 22, 2022
@asnaith
Copy link
Member

asnaith commented Feb 22, 2022

Looking good for the most part on the files I tested but I did find a few issues (1 of which is pre-existing though so I've logged a separate issue for it).

  1. One issue that is an introduction is the text size options on mobile - they're not working as intended. It looks like we don't show the increase/decrease options on our current release so perhaps the fix is just to omit them from being displayed on mobile?

Video:

RPReplay_Final1645561086.MP4
  1. Another introduction is specific to iOS. When I opened an .mp4 and then tried to close the preview modal it would show me empty video controls from the OS instead of the drive page. I recorded a video of the actual interaction because it's easier to see rather than on a screen recording. It doesn't do this on iOS with the current stage / prod builds.

Video:

video-preview-issue-ios.mov

@FSM1
Copy link
Contributor Author

FSM1 commented Feb 23, 2022

  1. One issue that is an introduction is the text size options on mobile - they're not working as intended. It looks like we don't show the increase/decrease options on our current release so perhaps the fix is just to omit them from being displayed on mobile?

Resolved this. Great catch.

  1. Another introduction is specific to iOS. When I opened an .mp4 and then tried to close the preview modal it would show me empty video controls from the OS instead of the drive page. I recorded a video of the actual interaction because it's easier to see rather than on a screen recording. It doesn't do this on iOS with the current stage / prod builds.

The video uploaded does not seem to be showing anything. Which browser were you using on iOS when testing this? There were no changes made to the video preview so I am not too sure why the behavior would be any different.

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Working great on all formats I could think of.

Copy link
Member

@asnaith asnaith left a comment

Choose a reason for hiding this comment

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

As we discussed in planning, I've created a separate issue for the iOS / Safari video issue #1970.

Everything else looks good.

@FSM1 FSM1 merged commit bb666c5 into dev Feb 23, 2022
@FSM1 FSM1 deleted the feat/file-preview-updates-1909 branch February 23, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

General feedback on the file preview
5 participants