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

updated the subscribe button styling to match the new youtube ui #29

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

gautamkrishnar
Copy link
Contributor

@gautamkrishnar gautamkrishnar commented Oct 31, 2023

Before

Screenshot 2023-10-31 at 10 18 50 PM

After

Screenshot 2023-10-31 at 10 18 18 PM

cc: @@bbilly1

@bbilly1
Copy link
Member

bbilly1 commented Nov 1, 2023

Looks good. Heads up, that is also used on the video detail page, e.g. /watch?v=, that's where the adjustOwner comes into play, as otherwise our button will overlap with the YT button, especially on smaller screens.

@gautamkrishnar
Copy link
Contributor Author

gautamkrishnar commented Nov 1, 2023

@bbilly1 good find 😄 fixed it.

Screenshot 2023-11-01 at 3 21 49 PM

Do you still want to keep the old code of adjustOwner that sets styles, because this new one is also looking good on small screens:
Screenshot 2023-11-01 at 3 26 21 PM

@bbilly1
Copy link
Member

bbilly1 commented Nov 2, 2023

Nice, looks like the adjust owner thing isn't really needed any more, I remember needing it before as this would mess up the positioning with the other buttons there.

I think there is still a small problem introduced with my changes from before, trying to build links for /shorts/ urls, but I don't think that's related to your changes here.

In any case, thanks for your improvements, let's merge this and I'll look into fixing the that on my side.

@bbilly1 bbilly1 merged commit 160580a into tubearchivist:master Nov 2, 2023
1 check passed
@gautamkrishnar gautamkrishnar deleted the subscribe branch November 2, 2023 13:38
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.

2 participants