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.

Sign up for GitHub

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

Jump to bottom

Feature/new window right click #2027

Merged
merged 14 commits into from Sep 22, 2022
Merged

Feature/new window right click #2027

merged 14 commits into from Sep 22, 2022

Conversation

constraintAutomaton
Copy link
Contributor

@constraintAutomaton constraintAutomaton commented Jan 25, 2022 •
edited

Feature/new window right click

Pull Request Type

  • Bugfix
  • Feature Implementation

Related issue
closes #2000

Description
When right-clicking a link, a video or a channel image a context menu appears giving the option to open the source into a new window.

Screenshots
Screenshot from 2022-01-25 18-02-13

Screenshot from 2022-01-25 18-02-47

Screenshot from 2022-01-25 18-02-56

Screenshot from 2022-01-25 18-07-48

Screenshot from 2022-01-25 18-07-55

Testing
Right click on a link or video and check if it is open in a new window.

Desktop

  • OS: Ubuntu
  • OS Version: 20.04.3 LTS
  • FreeTube version: v0.15.1 Beta

Additional context

@PrestonN PrestonN enabled auto-merge (squash) January 25, 2022 23:13
@PikachuEXE
Copy link
Collaborator

I think this PR will conflict with createWindow change in #1895
as I updated that function to accept arguments in object

Not sure why that PR takes so long to be reviewed :S

@PikachuEXE PikachuEXE mentioned this pull request Jan 26, 2022
Merged
2 tasks
@constraintAutomaton
Copy link
Contributor Author

constraintAutomaton commented Jan 26, 2022 •
edited

I think this PR will conflict with createWindow change in #1895 as I updated that function to accept arguments in object

Not sure why that PR takes so long to be reviewed :S

when your pr will be merged I will use your more comprehensive/elegant implementation of createWindow

@PikachuEXE
Copy link
Collaborator

I prefer that style much more than positional arguments especially when there are many arguments / optional arguments
https://simonsmith.io/destructuring-objects-as-function-parameters-in-es6

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

When i middle click on an yt url it doesnt open a new window. Would like to see that instead of opening it in the same window.

Maybe it's too much to ask in this PR but is it possible for u to fix this behavior in this PR. Closes #1923 (fixing this PR is partially covered by Pikachu in #1895 )

@constraintAutomaton
Copy link
Contributor Author

constraintAutomaton commented Jan 28, 2022 •
edited

When i middle click on an yt url it doesnt open a new window. Would like to see that instead of opening it in the same window.

Maybe it's too much to ask in this PR but is it possible for u to fix this behavior in this PR. Closes #1923 (fixing this PR is partially covered by Pikachu in #1895 )

I can fix it too (open in new window a youtube link in the description using right click), I'm not sure I understand what is in parathesis.

@PikachuEXE
Copy link
Collaborator

I am developing the fix already, figuring out how to handle query on new window
That fix deserves a separate PR
Also that fix I am developing depends on #1895

@PikachuEXE
Copy link
Collaborator

A bigger problem is a few PR stuck by not having a 3rd reviewer =_=|||

@constraintAutomaton
Copy link
Contributor Author

constraintAutomaton commented Jan 28, 2022 •
edited

I am developing the fix already, figuring out how to handle query on new window That fix deserves a separate PR Also that fix I am developing depends on #1895

ok, I let you do it and I will wait for the reviews.

auto-merge was automatically disabled February 6, 2022 22:20

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) February 6, 2022 22:21
@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 Feb 6, 2022
@PikachuEXE
Copy link
Collaborator

When i middle click on an yt url it doesnt open a new window.

@efb4f5ff-1298-471a-8973-3d47447115dc @constraintAutomaton
Fix submitted as #2083
Test it with more types of links though

efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc previously approved these changes Feb 21, 2022
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

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

Everything seems to work fine. LGTM

@PikachuEXE
Copy link
Collaborator

@efb4f5ff-1298-471a-8973-3d47447115dc
I guess you mean #2083?

Kind of confusing when I first read your last message here...

@PikachuEXE
Copy link
Collaborator

Will review remaining PR this week... or next
Busy!!

auto-merge was automatically disabled September 15, 2022 17:33

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) September 15, 2022 17:34
@ChunkyProgrammer
Copy link
Member

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

it's done, but I don't understand how to fix Lint code Base github action

we'll be updating that action soon. Don't worry about making it pass for the time being

PikachuEXE
PikachuEXE previously approved these changes Sep 16, 2022
View reviewed changes
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Tested locally

@PikachuEXE
Copy link
Collaborator

@ChunkyProgrammer @efb4f5ff-1298-471a-8973-3d47447115dc @absidue ?

absidue
absidue previously approved these changes Sep 19, 2022
View reviewed changes
@absidue absidue requested review from PrestonN and removed request for PrestonN September 19, 2022 10:22
@MarmadileManteater
Copy link
Contributor

By making all of the channel names in the slide out menu into anchors, the text color and text-style changed. This could be fixed with a little CSS.
0.17.1v:
image
This PR:

@PikachuEXE
Copy link
Collaborator

Just fixed it

@constraintAutomaton
Copy link
Contributor Author

Just fixed it

thank you!

@PikachuEXE
Copy link
Collaborator

@ChunkyProgrammer @absidue Please review 👋

@absidue
Copy link
Member

absidue commented Sep 20, 2022

@PikachuEXE we need @PrestonN to approve (even if we approve it won't be merged until he approves it as he requested changes)

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

Cant we dismiss his review somehow?

@PikachuEXE
Copy link
Collaborator

Try approving and see if it's auto merged
I don't see change requested on reviewer list
image

ChunkyProgrammer
ChunkyProgrammer approved these changes Sep 20, 2022
View reviewed changes
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer left a comment •
edited

Choose a reason for hiding this comment

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

It already had 3 reviews at one point so like absidue said, it's probably not gonna merge

Edit:
screensnip

@constraintAutomaton
Copy link
Contributor Author

Will he comeback "soon" or should I close the PR and create a new one?

@PrestonN PrestonN merged commit 420a91a into FreeTubeApp:development Sep 22, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer approved these changes

@PrestonN PrestonN Awaiting requested review from PrestonN

@absidue absidue Awaiting requested review from absidue

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

Successfully merging this pull request may close these issues.

[Feature Request]: right-click menu to open a channel or video in a new window
7 participants
@constraintAutomaton @PikachuEXE @efb4f5ff-1298-471a-8973-3d47447115dc @ChunkyProgrammer @MarmadileManteater @absidue @PrestonN