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.

Sign up for GitHub

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

Jump to bottom

Improve ui-ux on the stored videos modal #874

Merged
merged 5 commits into from
Apr 12, 2024
Merged

Improve ui-ux on the stored videos modal #874

merged 5 commits into from
Apr 12, 2024

Conversation

ArturoManzoli
Copy link
Contributor

Actions for multi-file selection:

  • CTRL+Click or
  • Long click 500 ms+

FIX #824

rafaellehmkuhl
rafaellehmkuhl requested changes Apr 9, 2024
View reviewed changes
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice interface! Loved it!

One thing that was not very clear to me was the confirmation button for deleting. I've tried deleting a few times, thinking it was not working, and then I realized I had to click the checkmark to continue. Could we make it more clear? Maybe instead of just the confirmation button, adding also a Cancel button, and having the actions named, would make it more clear.

More from the meeting:

  • Selecting multiple files today is not so intuitive
  • Selecting a third file is also requiring holding the file, which is different from the usual mobile approach
  • We are missing the "select all" button
  • Using "control" on Mac to select multiple is opening the context menu
  • Clicking the X on the snackbar to discard it is causing the video menu to close
  • When my CPU got hotter from using Cockpit and making the video call at the same time, trying to open the video menu caused a buggy situation where the whole menu was broken with a null video running
  • The placeholder images are coming from the internet, so they will be broken on offline situations

patrickelectric
patrickelectric requested changes Apr 9, 2024
View reviewed changes
Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

The UI looks great, I believe that is good at it's and we can continue improving it.
The CTRL+SHIFT does not look a common shortcut to me, maybe only shift is enough.
The Holding in each item is weird was well, I expect to hold only once and select as many videos as I want.

src/components/VideoLibraryModal.vue Outdated
</div>
<v-divider vertical class="h-[92%] mt-4 opacity-[0.08]"></v-divider>
<!--------------------------->
<!-- TAB for Video Button -->
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of such comments where the comment information is clearly a duplicated of the information bellow it.
In this case:

<!-- TAB for Video Button  -->
<template v-if="currentTab === 'videos'">

src/components/VideoLibraryModal.vue Outdated
Comment on lines 62 to 67
:class="[
selectedVideos.find((v) => v.filename === video.filename)
? 'border-opacity-[0.4]'
: 'border-opacity-[0.1]',
selectedVideos.find((v) => v.filename === video.filename) ? 'w-[220px]' : 'w-[190px]',
]"
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we use something like ?

Suggested change
:class="[
selectedVideos.find((v) => v.filename === video.filename)
? 'border-opacity-[0.4]'
: 'border-opacity-[0.1]',
selectedVideos.find((v) => v.filename === video.filename) ? 'w-[220px]' : 'w-[190px]',
]"
:class="selectedVideos.find((v) => v.filename === video.filename)
? ['border-opacity-[0.4]', 'w-[220px]']
: ['border-opacity-[0.1]', 'w-[190px]']
"

src/components/VideoLibraryModal.vue
}

const parseDateFromTitle = (title: string): string => {
const dateRegex = /\(([^)]+)\)/
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a comment here explaining what this magic regex is doing/parsing/extracting ?

src/components/VideoLibraryModal.vue Outdated
}, 500)
}

// Selects video on long press
Copy link
Member

Choose a reason for hiding this comment

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

This is another example where the comment is an duplication of the code.
For comments, I usually recommend this: https://www.youtube.com/watch?v=NLebZ3XT92E a must watch

src/components/VideoLibraryModal.vue Outdated
const index = selectedVideos.value.findIndex((v) => v.filename === video.filename)
if (index > -1 && selectedVideos.value.length === 1) {
return
} else if (index > -1) {
Copy link
Member

Choose a reason for hiding this comment

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

else is unnecessary after return

src/components/VideoLibraryModal.vue Outdated
const index = selectedVideos.value.findIndex((v) => v.filename === video.filename)
if (index > -1 && selectedVideos.value.length === 1) {
return
} else if (index > -1) {
Copy link
Member

Choose a reason for hiding this comment

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

else is unnecessary after return

@rafaellehmkuhl rafaellehmkuhl mentioned this pull request Apr 10, 2024
Closed
@ArturoManzoli ArturoManzoli force-pushed the 824-Improve-UI-UX-on-the-stored-videos-modal branch 2 times, most recently from fd84245 to 85a9d92 Compare April 11, 2024 17:42
@ArturoManzoli
Copy link
Contributor Author

On last update:

  • On Mac, now the key for multi-selection is 'Command';
  • Select all and select none buttons now present on multi-selection mode;
  • Multi-selection with a long-press/click is now analogue to android's material gestures using a gesture library (Hammer.js);
  • Confirmation for delete button is now more visible;
  • Implemented suggestions from last comments;
  • Dialog is now persistent, preventing closing from unwanted backdrop clicks;
  • Videos don't auto play anymore according to UI best practices;

rafaellehmkuhl
rafaellehmkuhl requested changes Apr 11, 2024
View reviewed changes
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is getting very good!

The only "bug" I found was that when you select more than one video, you can't unselect the last one by clicking it, like usual. One needs to click the "unselect all" button.

Screen.Recording.2024-04-11.at.15.01.49.mov

About the overall behavior, I still have my doubts around the "hold to select". It is intuitive on mobile, but not at all in desktop (which are our main customers).
Maybe we can have a "select" button, like in iOS, or show radio buttons on the left of the thumbnails, like in most UI? We can also discuss that on the meeting today.

@joaoantoniocardoso
Copy link
Member

Performance-wise, here on my machine it seems all good (RTX 3080 GPU):

With the backdrop-filter enabled:
image

With the backdrop-filter disabled:
image

The backdrop filter is being applied over the fonts/icons/video:

Enabled:
image

Disabled:
image

Ctrl-click on the current video:

  • flickers the view
  • selects the video name text
ctrl_click_current_video.mp4

@joaoantoniocardoso
Copy link
Member

And also.. Each icon seems to have a different size. Is this on purpose?

@ArturoManzoli ArturoManzoli force-pushed the 824-Improve-UI-UX-on-the-stored-videos-modal branch from 85a9d92 to 9552048 Compare April 12, 2024 11:37
rafaellehmkuhl
rafaellehmkuhl approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

Everything looks very good! Nice change!

@rafaellehmkuhl rafaellehmkuhl enabled auto-merge (rebase) April 12, 2024 12:15
@rafaellehmkuhl rafaellehmkuhl disabled auto-merge April 12, 2024 12:15
@ArturoManzoli ArturoManzoli force-pushed the 824-Improve-UI-UX-on-the-stored-videos-modal branch from 9552048 to 33d3295 Compare April 12, 2024 12:20
@ArturoManzoli ArturoManzoli force-pushed the 824-Improve-UI-UX-on-the-stored-videos-modal branch from 33d3295 to 4bd5d3d Compare April 12, 2024 13:57
@rafaellehmkuhl rafaellehmkuhl merged commit 605c234 into bluerobotics:master Apr 12, 2024
8 checks passed
@ArturoManzoli ArturoManzoli mentioned this pull request Apr 12, 2024
Closed
@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Apr 15, 2024
@ArturoManzoli ArturoManzoli deleted the 824-Improve-UI-UX-on-the-stored-videos-modal branch May 24, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaellehmkuhl rafaellehmkuhl rafaellehmkuhl approved these changes

@Williangalvani Williangalvani Awaiting requested review from Williangalvani

@joaoantoniocardoso joaoantoniocardoso Awaiting requested review from joaoantoniocardoso

@patrickelectric patrickelectric Awaiting requested review from patrickelectric

Assignees
No one assigned
Labels
docs-needed Change needs to be documented
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve the UI/UX on the stored videos (video configurations) page
5 participants
@ArturoManzoli @joaoantoniocardoso @patrickelectric @rafaellehmkuhl @ES-Alexander