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

* Implement divider for video "more dropdown menu" #1748

Merged
merged 7 commits into from
May 29, 2022

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Sep 24, 2021

Pull Request Type
Please select what type of pull request this is:

  • Feature Implementation

Related issue
IRC chat:

Non-pressing question: if we were to ever have breakout sections on hover of the three dots video section dropdown, would the better organization be "Open >" & "Copy >" or "YouTube >" & "Invidious >"?

Description

  • Update the dropdown opened by a button with title "More Options",
    located on video card/box rendered in views like search result/history/trending,
    so that the options are reordered and group together.
    Dividers are used for grouping.
  • Menu item spacing reduced slightly.

Screenshots (if appropriate)
New screenshots (all dropdown updated by this PR, too lazy to take for all themes)
image
image
image

Old screenshots (before divider opacity reduced):
image
image
image
image

Testing (for code that is not small enough to be easily understandable)
A: More Options

  • Visit Trending
  • Click "More Options" button
  • Click on each item to if works as labeled

B: Download Video

  • View a video (e.g. https://youtu.be/vyuJRJ6GQ5k)
  • Click "Download Video" button
  • Click on each item to if works as labeled (I only tested limited options)

C: Change Video Formats

Desktop (please complete the following information):

  • OS: MacOS
  • OS Version: 11.6
  • FreeTube version: 732b2d6

Additional context
Add any other context about the problem here.

@PrestonN PrestonN enabled auto-merge (squash) September 24, 2021 05:43
@PikachuEXE
Copy link
Collaborator Author

PikachuEXE commented Sep 24, 2021

dropdownValues & dropdownNames should be removed eventually
Just not sure if you want me to update the other 2 places using ft-icon-button to use the new API (i.e. more code changes to review)

Edit: This probably won't make into 0.15 release, no need to rush

@efb4f5ff-1298-471a-8973-3d47447115dc

IIRC there is only one more dropdown like this

FreeTube_wWk5jfbACw

@PikachuEXE
Copy link
Collaborator Author

2 more

  • The download one you shown
  • The video format one (DASH/Legacy/Audio) on the right of download button you pressed

@PikachuEXE
Copy link
Collaborator Author

Updated all existing ft-icon-button users to use new API, updated PR description & screenshots about that
Not tested in all themes after divider style fix

@ChunkyProgrammer ChunkyProgrammer added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 24, 2021
@efb4f5ff-1298-471a-8973-3d47447115dc

found another dropdown https://youtube.com/playlist?list=PLF1CC814A59FF59DE

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 3, 2021
@PikachuEXE
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc
I assume you are referring to the button shown in the following screenshot
And it's using ft-list-dropdown instead of ft-icon-button thus there is no need to update it

image

@PikachuEXE PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Oct 4, 2021
@PikachuEXE PikachuEXE force-pushed the temp/video-menu-reorder branch from 89b8892 to 645f3c7 Compare October 4, 2021 02:51
@ChunkyProgrammer ChunkyProgrammer mentioned this pull request Oct 12, 2021
19 tasks
@peepo5
Copy link
Contributor

peepo5 commented Oct 12, 2021

I feel like this is an improvement, but it is a workaround for universal new share menu implimentation:
#1450

@PikachuEXE
Copy link
Collaborator Author

But #1450 does not mention anything for "Mark as Watched"

@peepo5
Copy link
Contributor

peepo5 commented Oct 12, 2021

that could be a seperate button centered at the top

Copy link
Member

Choose a reason for hiding this comment

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

LGTM! Only thing i might change is in Dark mode it is hard to that an option is highlighted when u hover over it.

@PikachuEXE
Copy link
Collaborator Author

Well I don't have good idea to fix that and that problem is not specific to this PR

@PikachuEXE
Copy link
Collaborator Author

In progress (just restarted today)
Will take some time
Those code conflicts~~~
Well those actually resolved but now it's bugged lol

@PikachuEXE
Copy link
Collaborator Author

Merge conflict fixed and whatever bug(s) I found fixed
But I have only tested the dropdown in screenshots (and I only test them briefly)
So please see if there are any more dropdowns that should be tested

@PikachuEXE PikachuEXE requested review from absidue and removed request for peepo5, kommunarr, Hiers and Svallinn May 25, 2022 06:19
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: merge conflicts / rebase needed labels May 25, 2022
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

The reduction of the vertical padding around the menu options combined with the dividers make the longer options look quite cramped.

bit_cramped

I think 8px might be a good comprimise that still looks quite good without taking up too much space:
8px

Here is what it looks like with 10px:
10px

@PikachuEXE
Copy link
Collaborator Author

@absidue
8px now
Any functionality related bug found?

@absidue
Copy link
Member

absidue commented May 29, 2022

I didn't find any functionality issues, thanks for updating the padding.

Copy link
Member

@ChunkyProgrammer ChunkyProgrammer left a comment

Choose a reason for hiding this comment

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

LGTM

@PrestonN PrestonN merged commit 713738b into FreeTubeApp:development May 29, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 29, 2022
@PikachuEXE PikachuEXE deleted the temp/video-menu-reorder branch May 30, 2022 01:45
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.

6 participants