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

Add toggle to export playlist for older FT #62

Conversation

PikachuEXE
Copy link
Owner

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Addition to FreeTubeApp#4234

Description

New toggle to allow user to export playlists in a format consumable by old versions of FT

Screenshots

image

Testing

  • Checkout this PR, add some playlists and videos, probably also removed the default playlists
  • Export playlists as usual (also as backup file for different tests
  • Export playlists with new toggle on
  • Remove all playlists
  • Switch back to development, import the export with new toggle on
  • Confirm all videos from all playlists are present

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@kommunarr
Copy link

kommunarr commented Nov 22, 2023

Haven't tested yet, but here's my suggestion on the tooltip text:

This option exports all playlist videos into one Favorites playlist.
To use your current playlists in an older version of FreeTube:
1. Export your playlists using this option.
2. Delete all of your existing playlists using the Remove All Playlists option under Privacy Settings.
3. Load up the older version of FreeTube and import your playlists.

@PikachuEXE
Copy link
Owner Author

Counter suggestion

This option exports videos from all playlists into one playlist named Favorites.
How to export & import videos in playlists for an older version of FreeTube:

  1. Export your playlists with this option enabled.
  2. Delete all of your existing playlists using the Remove All Playlists option under Privacy Settings.
  3. Launch the older version of FreeTube and import the exported playlists.

@kommunarr
Copy link

Sounds good. Try to keep the line breaks in the tooltip if possible, but it's ok if not

@PikachuEXE
Copy link
Owner Author

PikachuEXE commented Nov 22, 2023

Try to keep the line breaks in the tooltip if possible

How to do that, any existing example?

@PikachuEXE
Copy link
Owner Author

No existing example, need to create new option to allow new lines
image

@kommunarr
Copy link

This looks good, although it still is not very easy to follow the bullets when they're centered. Would it be too much to ask to have the text be start or end aligned as well?

@PikachuEXE
Copy link
Owner Author

I guess I will just make them all left align (when new line enabled)
Wondering if all tooltips should be converted to enable new line later (out of scope)

@kommunarr
Copy link

Good thinking, I think that should be fine as long as we're intentional about how we use the block style indicator for any other tooltips (currently just the External Link Handling one AFAICS).

@PikachuEXE
Copy link
Owner Author

Updated
image

@PikachuEXE
Copy link
Owner Author

Fixed the annoying extra surrounding spaces
image

Label: Export Playlists For Older FreeTube Versions
# |- = Keep newlines, No newline at end
Tooltip: |-
This option exports videos from all playlists into one playlist named `Favorites`.

Choose a reason for hiding this comment

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

nitpick: use quotes, not backticks

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done
image

@kommunarr
Copy link

kommunarr commented Nov 23, 2023

nitpick: new pattern especially necessitates max fixed width (as % or vw). Edit: or as fixed max width in px. Would have to check our tooltip code to see how it's being done now, but it looks to stay pretty consistently constrained width-wise across our different tooltips.

Screenshot_20231122_183858

})
})

await this.promptAndWriteToFile(options, JSON.stringify([favoritesPlaylistData]), 'All playlists has been successfully exported')

Choose a reason for hiding this comment

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

nitpick: String needs to be localized (multiple instances)

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's a translation key suffix
image

Copy link

@kommunarr kommunarr Nov 23, 2023

Choose a reason for hiding this comment

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

Thanks, didn't see that - I forgot about this from Chunky's recent linting PR - no problem here


exportPlaylistsForOlderVersions: async function () {
const dateStr = getTodayDateStrLocalTimezone()
const exportFileName = 'freetube-playlists-for-single-favorites-playlist-' + dateStr + '.db'

Choose a reason for hiding this comment

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

suggestion (non-blocking):

Suggested change
const exportFileName = 'freetube-playlists-for-single-favorites-playlist-' + dateStr + '.db'
const exportFileName = 'freetube-playlists-as-single-favorites-playlist-' + dateStr + '.db'

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done

@kommunarr
Copy link

Testing: worked well for me! Handled multiple playlists with multiple overlapping duplicates. Using the instructions, it was imported to my nightly build without any duplicates or issues.

@PikachuEXE
Copy link
Owner Author

PikachuEXE commented Nov 23, 2023

Breakpoint now set at 1000px to wrap
image
image

Temp note for myself: https://gomakethings.com/how-to-check-if-any-part-of-an-element-is-out-of-the-viewport-with-vanilla-js/

@kommunarr
Copy link

issue (non-blocking) Still seems risky with other more verbose languages (demonstration below). But not a blocking issue if we want to put a pin in that.

Screenshot_20231122_200757

@PikachuEXE
Copy link
Owner Author

Just made tooltip auto-wrap

Screenshot 2023-11-23 at 11 41 11
Screenshot 2023-11-23 at 11 41 22

@kommunarr
Copy link

Not sure where this was from but it seems a bit overengineered and isn't working right on some devices (tested with iPhone XR devtool). Why not just use a max-inline-size or something to that effect?

Screenshot_20231122_221325

@kommunarr
Copy link

Even just this without the media query looks fine:

  white-space: pre-wrap;
  text-align: start;
  min-inline-size: 55vw;
}

@kommunarr
Copy link

Here's my suggestion: #63

@PikachuEXE
Copy link
Owner Author

I rarely used vw to know how to use them properly -_-

Copy link

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

It's not too often honestly, but it makes sense here with us trying to keep an absolutely positioned popup within dynamic screen constraints. LGTM.

@PikachuEXE PikachuEXE merged commit dbe57a4 into feature/playlist-2023-05 Nov 23, 2023
@PikachuEXE PikachuEXE deleted the feature/playlist-2023-05-w-old-ft-consumable-playlist-export branch December 5, 2023 06:13
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.

None yet

2 participants