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

A new way to subscribe #4238

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Oct 27, 2023

A New Way to Subscribe, Part II - An Even Newer Way to Subscribe: The Last of the Subscribe Buttons; A New Hope: The Final Chapter

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #1615
closes #568

Description

Modifies the humble ft-subscribe-button with — dare I say it — a new way to subscribe. Click to subscribe/unsubscribe and see every profile's subscribed/unsubscribed state. Click any of those profiles to subscribe/unsubscribe them from that channel. The old way can be enabled again by disabling the hideProfileDropdownToggle prop, but this PR does not use this manual override anywhere due to the lack of clear relative benefit it provides for any existing use cases.

Does not change the default behavior of All Channels (if subscribing to a channel with another profile, All Channels automatically subscribes to it; if unsubscribing to a Channel with the All Channels profile, unsubscribe to it from all profiles). Due to its special functionality, All Channels and the currently active profile are always top of the list. Then goes unsubscribed profiles, then subscribed profiles.

Also fixes aria-expanded for FT dropdowns.
Also removes vendor-specific prefixes for transition property at @absidue's request. (This is safe.)

Screenshots

Screenshot:
Screenshot_20231027_000839

Video:

simplescreenrecorder-2023-09-20_19.22.02-2023-10-27_00.13.26.mp4

Testing

  • Test that All Channels works as described in description (i.e., how it currently works)
  • Test that profiles are successfully subscribed/unsubscribed
  • Test that sort order of profiles is as described in description
  • Test keyboard navigation of profile list and activation through space bar
  • Test on emulated mobile view
  • Test that the old way still looks good (enable prop in, e.g., its usage in SubscribedChannels.vue)

Desktop

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

Additional notes

One main decision point is whether to open the dropdown when subscribe/unsubscribe is clicked. I went with "yes" because none of the dropdown locations are very obtrusive, so it's basically cost-free to save the click.

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 27, 2023 05:57
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 27, 2023
@kommunarr kommunarr force-pushed the feat/new-way-to-subscribe branch from 920cbbe to b8f0c2c Compare October 27, 2023 06:10
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Oct 27, 2023

Doesn't this also closes #568

I think we can also close #1934 in favor of this

@kommunarr kommunarr force-pushed the feat/new-way-to-subscribe branch 2 times, most recently from f381640 to aa4795e Compare October 27, 2023 13:14
Note: was able to add aria-controls to ft-profile-selector because it keeps the hidden dropdown in the DOM. The same is not true of the ft-icon-button or ft-subscribe-button. Main point: aria-expanded should go on the button opening the dropdown, not the dropdown itself.
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 solution for the bug you mentioned in the "Additional context" section, is to do this change in the SubscribedChannels.vue component:

<div
  v-for="channel in channelList"
-  :key="channel.key"
+  :key="channel.id"
  class="channel"
>

As the .key property doesn't exist on the channel object, we are currently passing undefined to :key, which is the same as not passing a key at all. Vue tries to reuse as much of the existing DOM as possible when doing updates, as we aren't giving it any hints on what to remove and what to keep, it ends up reusing the removed channel bubble. By adding the key, Vue then knows to invalidate and remove the node with that key, but it also means that it can keep all the nodes who's keys haven't changed.

@kommunarr kommunarr requested a review from absidue October 28, 2023 00:05
<li
v-for="(profile, index) in profileDisplayList"
:id="'profile-' + index"
:key="'subscription-profile-' + index"
Copy link
Member

@absidue absidue Oct 28, 2023

Choose a reason for hiding this comment

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

Could you please change the id's and key to use the profile _id instead of the index, also the key can be simplified to just use the _id, as it is just used by Vue inside this component and isn't added to the DOM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, will do; I admittedly was just copying the pattern used by the ft-profile-selector. I was unsure of whether this could cause conflict if we had other elements in the DOM with the same key values (e.g., if we changed the ft-profile-selector accordingly), but I'm pretty sure that is not a major issue after checking up on the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

For the id you should still use globally unique names, as those get placed into the DOM, but keys are only used for Vue processing in the scope of a component, so those don't have to have a unique prefix or anything like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm... for some reason using the _id as key breaks focusout, even though it's populating properly... I am going to keep using the index + a specific prefix for the id and key (i.e., subscription-profile-${index} in light of this. I don't see any harm in it, and we do need unique prefixes as stated for the id if we're to have any other similar usage of profile._ids.

@kommunarr kommunarr force-pushed the feat/new-way-to-subscribe branch from f630349 to 5b5c773 Compare October 28, 2023 15:44
@kommunarr kommunarr force-pushed the feat/new-way-to-subscribe branch from 5b5c773 to 0f3ae7a Compare October 28, 2023 15:59
Copy link
Member

Choose a reason for hiding this comment

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

All Channels and the currently active profile are always top of the list. Then goes unsubscribed profiles, then subscribed profiles.

This is not the case when u subscribe by clicking the subscribe button on a non All Channels profile

VirtualBoxVM_YifFRXSqeb.mp4

@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 Nov 22, 2023
@kommunarr
Copy link
Collaborator Author

All Channels and the currently active profile are always top of the list. Then goes unsubscribed profiles, then subscribed profiles.

This is not the case when u subscribe by clicking the subscribe button on a non All Channels profile

Not sure I see the issue. Currently active profile is shown after All Channels for convenience as well.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, i didn't read well :(

LGTM

@FreeTubeBot FreeTubeBot merged commit e68c534 into FreeTubeApp:development Nov 22, 2023
5 checks passed
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Nov 23, 2023
* development:
  Translated using Weblate (Italian)
  Translated using Weblate (Polish)
  Translated using Weblate (Kurdish (Central))
  Translated using Weblate (Spanish)
  Translated using Weblate (Kurdish (Central))
  Translated using Weblate (Kurdish)
  A new way to subscribe (FreeTubeApp#4238)
  Display more profiles on the profile drop-down list, v2 (fixed for low res, fixed linter)  (FreeTubeApp#4359)
  Translated using Weblate (Estonian)
  Translated using Weblate (Serbian)
  Translated using Weblate (Arabic)
  Translated using Weblate (Chinese (Simplified))
  Display currently watching viewer count on live streams (FreeTubeApp#4206)
  Translated using Weblate (Spanish)
  Translated using Weblate (Czech)
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Italian)
  Fix block channel channel ID validation (FreeTubeApp#4366)
@efb4f5ff-1298-471a-8973-3d47447115dc

Sorry i forgot to test this page

I dont think it makes sense that the dropdown expands right before the channel gets removed from the list. This behavior is good on every other instance the but on this page it seems weird to me

VirtualBoxVM_cQLA0F7Kdp.mp4

@kommunarr
Copy link
Collaborator Author

That's fair - functionally inconsequential but of note - will add as chore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants