Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #3976: Replace Toast with Playlist URL Bar Icon + Add Playlist Menu Item #3977

Merged
merged 6 commits into from
Aug 25, 2021
Merged

Fix #3976: Replace Toast with Playlist URL Bar Icon + Add Playlist Menu Item #3977

merged 6 commits into from
Aug 25, 2021

Conversation

Brandon-T
Copy link
Collaborator

@Brandon-T Brandon-T commented Jul 28, 2021 •
edited by iccub
Loading

Summary of Changes

  • Security Review: https://github.com/brave/security/issues/553

  • Removes Toast completed

  • Adds Playlist URL Bar Button + Popover

  • Adds Playlist to Tabs so each tab determines when its own Playlist button is shown in the URL bar (similar to rewards button).

  • Add Playlist Menu Button and Menu Badge.

  • Hide Playlist icon if reader-mode takes precedence, otherwise hide reader-mode button if playlist URL is supported.

This pull request fixes #3976 && #4070

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

  • Test switching tabs while two tabs have media items.
  • Test two tabs that load media at the same time.
  • Test that the playlist URL bar button shows the correct icon.

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@Brandon-T Brandon-T force-pushed the feature/PlaylistURLBarButton branch 2 times, most recently from f1c37d3 to 1347936 Compare August 9, 2021 18:15
@Brandon-T Brandon-T force-pushed the feature/PlaylistURLBarButton branch 3 times, most recently from 08a8a1a to a1b7adf Compare August 13, 2021 15:36
iccub
iccub suggested changes Aug 16, 2021
View reviewed changes
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

code/nitpick review, going to run this branch and leave functional feedback later

Client/Frontend/Browser/BrowserViewController.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/BrowserViewController/BrowserViewController+ReaderMode.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/BrowserViewController/BrowserViewController+ToolbarDelegate.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/BrowserViewController/BrowserViewController+ToolbarDelegate.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/Playlist/BrowserViewController+Playlist.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/Playlist/PlaylistPopoverView.swift Show resolved Hide resolved
Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/PlaylistMenuButton.swift Outdated Show resolved Hide resolved
Client/Frontend/UserContent/UserScripts/Playlist.js Outdated Show resolved Hide resolved
Client/Frontend/Widgets/MenuButton.swift Show resolved Hide resolved
Client/Frontend/Widgets/MenuButton.swift Outdated Show resolved Hide resolved
@Brandon-T Brandon-T force-pushed the feature/PlaylistURLBarButton branch 2 times, most recently from e1c9a6e to 2e66cf2 Compare August 18, 2021 17:51
@soner-yuksel
Copy link
Contributor

soner-yuksel commented Aug 19, 2021 •
edited
Loading

f6863f0 NO!! 😢

1no1

@Brandon-T Brandon-T changed the title [Playlist] - Replace Toast with Playlist URL Bar Icon + Add Playlist Menu Item [Playlist - Fix #3976 && #4070] - Replace Toast with Playlist URL Bar Icon + Add Playlist Menu Item Aug 19, 2021
@Brandon-T Brandon-T changed the title [Playlist - Fix #3976 && #4070] - Replace Toast with Playlist URL Bar Icon + Add Playlist Menu Item [Playlist - #3976, #4070] - Replace Toast with Playlist URL Bar Icon + Add Playlist Menu Item Aug 19, 2021
@Brandon-T Brandon-T requested a review from iccub August 19, 2021 17:55
iccub
iccub suggested changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

2 small changes and rest looks good

BraveUI/SwiftUI/FontScaling.swift Outdated Show resolved Hide resolved
Client/Application/ClientPreferences.swift Outdated Show resolved Hide resolved
@iccub iccub mentioned this pull request Aug 23, 2021
Closed
3 tasks
@iccub iccub changed the title [Playlist - #3976, #4070] - Replace Toast with Playlist URL Bar Icon + Add Playlist Menu Item Fix #3976: Replace Toast with Playlist URL Bar Icon + Add Playlist Menu Item Aug 23, 2021
@Brandon-T
Added playlist button to URL Bar and Playlist tooltips.
8045bd7 
Remove PlaylistToast completely. Fixed Playlist URL Bar Icon display when switching tabs and when playlist items are detected.
Add Playlist Menu.
Dismiss menu properly.
Fixing glitch where web-view is somehow not removed when the tab is killed.
Integrated above readermode.
Adding ability to detect media element's information based on tagId.
Open playlist item at current time offset from page view offset.
Fixed reader-mode button state and playlist url bar not updating when switching tabs (sometimes).
Fixed onboarding bug to show automatically.
Show popover after adding to playlist automatically.
Added back toast!
Fixing page detection for some iFrames.
Addressed Feedback
Fix when closing a blank tab it destroys playlist url bar button.
Finished playback where left off fixes.
Hide the menu badge when the item is in the playlist.
@Brandon-T
Removed cryptographically because security doesn't endorse it.
912afe8
@Brandon-T
Added comment explaining UUIDv4 algorithm.
803a5f8
@Brandon-T
Bug fixes.
6bc0e2a
@Brandon-T Brandon-T requested a review from iccub August 25, 2021 13:31
@iccub iccub added this to the 1.31 milestone Aug 25, 2021
@iccub iccub merged commit 998bb96 into development Aug 25, 2021
@iccub iccub deleted the feature/PlaylistURLBarButton branch August 25, 2021 16:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iccub iccub iccub approved these changes

@kylehickinson kylehickinson Awaiting requested review from kylehickinson

@soner-yuksel soner-yuksel Awaiting requested review from soner-yuksel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.31
Development

Successfully merging this pull request may close these issues.

Playlist - Replace Toast with URL Bar Icon + Add Playlist Menu Button
3 participants
@Brandon-T @soner-yuksel @iccub