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

List sorting & display settings #4231

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Oct 25, 2023

List sorting & display settings

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

#4096, comment

Description

  • Implements radio group support for ft-icon-button dropdown
  • Implements ft-button option for ft-icon-button base button appearance
  • Implements three buttons in Theme Settings for fitting three of our features with new capabilities:
    1. Settings Sections,
      • Use default sort (default), sort A to Z, sort Z to A
    2. Side Nav subscription bubbles, and
    3. the ft-profile-selector dropdown:
    • Sort A to Z, sort Z to A
    • Display type: list or grid view
      • (grid-specific): Number of items per grid row
      • (grid-specific): Show or hide titles

Video

This video demos everything listed in the Testing section:

simplescreenrecorder-2023-09-20_19.22.02-2023-10-25_03.41.56.mp4

Edit: After minor updates to settings placement and changing the settings section sort:
Screenshot_20231026_144250

Testing

  • Test radio group functionality of the ft-icon-button dropdown:
    • Working up/left/down/right arrow navigation
    • Tab / Shift + Tab brings you out of the radio group to the next / previous tabbable element
    • Spacebar checks the focused checkbox
  • Check that the expanded side nav, settings section, and profile selector dropdown behave normally with settings.js defaults
  • Check that side nav subscriptions look good with every combination of grid, itemsPerGridRow, and showTitles and respects the selected sorting order
  • Check that the profile selector looks good with every combination of grid, itemsPerGridRow, and showTitles and respects the selected sorting order
  • Check that the settings sections respect the selected sorting order

Desktop

  • OS: OpenSUSE Tumbleweed
  • OS Version: 2023xxxx
  • FreeTube version: 0.19.1

Additional context

Remaining related discussions: #1739, #987, #800, parts of #4096, #2871

The new radio group support in the ft-icon-button dropdown was just created as a helpful tool for bringing about what I was envisioning, but I can see it coming in handy again for creating related settings in a space-compressed and low-cognitive-complexity way. See discussion under #2295.

Among other reasons, I also created this when pondering the design of this issue that I intend to work on: #1615.

Please let me know how you feel about the default grid values chosen in settings.js (in scope for this PR) or if we would consider changing any of the default display values / sorting methods (broader discussion out of scope for this PR). I'm personally going to be using the grid + 3 per grid row + show titles for subscriptions and grid + 4 per grid row + show titles for the profile selector during my personal use of FreeTube.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 25, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 25, 2023 09:19
@kommunarr kommunarr removed the PR: WIP label Oct 25, 2023
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.

Not sure if you'll be doing any changes to this after the discussion on Matrix, but I thought I would do a code review on the pull request in it's current state anyway.

src/renderer/views/Settings/Settings.js Outdated Show resolved Hide resolved
src/renderer/helpers/utils.js Show resolved Hide resolved
src/renderer/components/side-nav/side-nav.vue Outdated Show resolved Hide resolved
src/renderer/components/ft-icon-button/ft-icon-button.vue Outdated Show resolved Hide resolved
src/renderer/views/Settings/Settings.vue Outdated Show resolved Hide resolved
src/renderer/components/side-nav/side-nav.vue Outdated Show resolved Hide resolved
kommunarr and others added 2 commits October 26, 2023 12:53
Co-authored-by: absidue <48293849+absidue@users.noreply.github.com>
@kommunarr kommunarr force-pushed the feat/add-list-display-settings branch from 8192d6b to 92aea64 Compare October 26, 2023 18:00
…iquez/FreeTube into feat/add-list-display-settings
@kommunarr kommunarr force-pushed the feat/add-list-display-settings branch from 92aea64 to 3f108f8 Compare October 26, 2023 18:03
@kommunarr kommunarr force-pushed the feat/add-list-display-settings branch from 5abe532 to 732fd29 Compare October 26, 2023 18:11
@kommunarr
Copy link
Collaborator Author

Implemented suggestions listed here and those discussed in the Element channel, including hiding these non-mobile-specific theme options on mobile, changing the settings section sort to be an ft-select, removing the Z to A sort option for the settings section.

Opening this back up for review.

…date settings section sort to be ft-select with only default and ascending options; adjust labels; adjust new controls' placement & appearance in settings
@kommunarr kommunarr force-pushed the feat/add-list-display-settings branch from 45de565 to dc28f86 Compare October 26, 2023 19:47
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 27, 2023
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Member

Choose a reason for hiding this comment

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

User without context doesn't know that they have to click on the hamburger menu on the top left to see the grid. If user selects grid and its not expanded already nothing happens from an user perspective. Should we enable Expand Side Bar by Default when grid is selected?

Also labels are gone when hovering when show grid item titles is enabled which i understand but allot of the titles are cut of when u have a grid of 5 so we might want to add them back because it could be hard for the user to know what channel they're looking at

VirtualBoxVM_OAA099OjjB.mp4

Copy link
Member

Choose a reason for hiding this comment

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

  • Profile name and titles are not centered when in grid view
  • Hovering over profiles in list mode doesnt show labels
  • Hovering over profiles in grid mode only shows labels when hide gride item titles is selected

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 22, 2023
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Apr 20, 2024

I remember this being a big discussion. I suppose it's not worth people's review time while we have so many bigger fish. In retrospect, I do wish I consulted with the team before working on this, as it seems to have stimulated some lively & silly debate, much of which was my own making. Things looking back at it in retrospect:

  • I don't like that the buttons don't fit into the rest of the Theme Settings very well. I think we should have more icons associated with our ft-inputs and what not, but I don't know about the layout on this one.

I was trying to come up with a bigger list, but honestly, I still like the idea behind these. I don't think it's a big deal to add more stuff to Theme Settings, as that's the most optional place, and where customizability to your heart's extent is fine, as long as the interface is good. I think the main problem is more just functional in that we have so many more important items, that this is hard to justify reviewers' time on this compared to most other PRs.

@kommunarr kommunarr added PR: low priority For pull requests that don't require a fast review. e.g. code cleanup PR: draft awaiting team consensus For a functionally complete PR whose implementation is still under discussion or expected to change. labels Apr 20, 2024
@kommunarr kommunarr marked this pull request as draft April 20, 2024 14:29
auto-merge was automatically disabled April 20, 2024 14:29

Pull request was converted to draft

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

Only thing i like to see removed is the setting section sort order as i think having better defaults in the sense that we have a logical order we display the settings in makes more sense than this imo

@kommunarr
Copy link
Collaborator Author

Only thing i like to see removed is the setting section sort order as i think having better defaults in the sense that we have a logical order we display the settings in makes more sense than this imo

I think having the option for alphabetical-based visual scanning is reasonable, because it's hard to have an "intuitive" ordering of those 14 sections

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 16, 2024

Only thing i like to see removed is the setting section sort order as i think having better defaults in the sense that we have a logical order we display the settings in makes more sense than this imo

I retract my previous statement. We have the Sort Settings Sections (A-Z) in the top of the settings for a while now and I do see the purpose of it. I would be fine If this gets moved into the theme settings or if it stays where it is now.

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

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

Remaining related discussions: #1739, #987, #800, parts of #4096, #2871

Please let me know how you feel about the default grid values chosen in settings.js (in scope for this PR) or if we would consider changing any of the default display values / sorting methods (broader discussion out of scope for this PR). I'm personally going to be using the grid + 3 per grid row + show titles for subscriptions and grid + 4 per grid row + show titles for the profile selector during my personal use of FreeTube.

  • I dont have any opinion on the values chosen as i wont use FT in this way. Visually it looks good though

@kommunarr
Copy link
Collaborator Author

Thanks for taking the time to take inventory of these @efb4f5ff-1298-471a-8973-3d47447115dc. I will say that I'm still feeling aligned with my earlier comment:

I think the main problem is more just functional in that we have so many more important items, that this is hard to justify reviewers' time on this compared to most other PRs.

For that reason, I am not likely to revive this, especially given that I have taken out the bits that I like from it into existing & closed PRs (see the PRs that reference this one). Given the large number of merge conflicts on this and how a good deal of it is already being incorporated piecemeal, I'm thinking any possibility of a continued implementation effort on this should start from a separate branch.

@kommunarr kommunarr closed this Oct 21, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc removed PR: low priority For pull requests that don't require a fast review. e.g. code cleanup PR: draft awaiting team consensus For a functionally complete PR whose implementation is still under discussion or expected to change. labels Oct 21, 2024
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