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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(twitter): X support for Twitter #894

Closed
wants to merge 2 commits into from

Conversation

ClaytonTDM
Copy link

馃敡 What does this fix? 馃敡

Adds support for x.com

馃棐 Checklist 馃棐

  • I have read and followed Catppuccin's contributing guidelines.
  • I have updated the version appropriately in the ==UserStyle== header of the catppuccin.user.css file.

@ClaytonTDM ClaytonTDM requested a review from watatomo as a code owner May 17, 2024 04:36
@github-actions github-actions bot added the twitter Twitter label May 17, 2024
@ClaytonTDM ClaytonTDM changed the title X support for Twitter fix: X support for Twitter May 17, 2024
@ClaytonTDM ClaytonTDM changed the title fix: X support for Twitter fix(twitter): X support for Twitter May 17, 2024
@nonetrix
Copy link
Contributor

Ah oops we made a PR at the same time lol, I think Twitter.com should be kept since Twitter control panel uses it. I guess I will close my PR since you where the first LGTM but the git diff is a little messy for some reason

@nonetrix nonetrix mentioned this pull request May 17, 2024
2 tasks
Copy link
Member

Choose a reason for hiding this comment

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

Could this entire change not just be. All I can see here is copy and paste of the entire "twitter.com" code into a new "x.com" domain/

- @-moz-document domain("twitter.com")
+ @-moz-document domain("twitter.com"), domain("x.com")

Copy link
Author

Choose a reason for hiding this comment

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

sorry i dont know a lot abt css, i just found not having x support annoying and had to make a pr ;-;

@@ -1028,6 +1780,143 @@
}
}

@-moz-document domain("x.com") {
Copy link
Member

Choose a reason for hiding this comment

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

Why select domain("x.com") twice when we already possess a instance of domain("x.com") once from above.

Copy link
Author

Choose a reason for hiding this comment

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

because there was a second twitter.com

Copy link
Contributor

Choose a reason for hiding this comment

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

Your right I missed that, there is one for light version too. I am sorry, but why is it this way anyway? Couldn't it be used twice? I made the PR really late and just assumed it was once in the code, I feel really dumb now that this has been merged

Copy link
Contributor

@nonetrix nonetrix May 17, 2024

Choose a reason for hiding this comment

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

Seems other themes don't do it that way, seems redundant :/

But I guess it could be considered cleaner, I feel like style guidelines should be made for stuff like this. I am not sure what everyone would prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed I am so sorry everyone #898

@nonetrix
Copy link
Contributor

nonetrix commented May 17, 2024

I can reopen my PR if everyone is cool with that, but I'd rather them get the credit to be honest:3

Eh whatever I'll just let everyone decide gn it's open again

@isabelroses
Copy link
Member

superseded by #895

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

Successfully merging this pull request may close these issues.

None yet

3 participants