-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Color change in the links and several improvements in the comments #334
Conversation
…e in the uploader comments and I finished permalink of the comments.
…om the uploader (plus additional classes in default.css). The isEdited attribute was also added in the comments API and new strings in en-US.json
assets/css/lighttheme.css
Outdated
@@ -4,6 +4,6 @@ a:active { | |||
} | |||
|
|||
a { | |||
color: #303030; | |||
color: #0093ff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something like #606060
be better here? I also think links should be more noticeable, but this makes every a
on the page a bright blue, so it's difficult to distinguish important information. Having only some links be this color (only links in the description, for example) would be better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not convinced by the color #0093ff
for all the links, I changed it to #61809b
that I think achieves a good balance between making the links more noticeable but without being too bright. What do you think? Otherwise I have no problem in leaving it as it was and highlight only the links of certain sections (such as description or comments).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this comment before merging. If you'd like to open a new PR or a new issue I'd be happy to look at some proposed changes or continue discussion. I think #61089b
looks fine though so I don't have a problem if you'd like to leave it.
Fantastic! I left a couple notes. Overall excellent and I'm excited to merge this. |
@aaferrari did you test your changes also with dark theme? |
@dimqua I've been testing it with a couple of videos and it looks good. The change of colors in the links are limited to the light theme except for the highlighting of the name of who uploaded the video that applies to both themes (although if you think that in the dark theme it would be better of another color to let me know). |
LGTM. 👍 |
Awesome! @aaferrari I suggest you to help omarroth with #118, if you have time and interest. It'd be really appreciated. |
This is a summary with the modifications:
assets/css/lighttheme.css
so that they are better distinguishable from the text. If you prefer you can omit this or use a different color.translate
function because it depends a lot on the country, but it would be missing some way of translate the days of the week and months according to the language of the user in a similar way to this.locales/en-US.json
.In the following screenshots you can better appreciate all the changes made:
assets/css/default.css
.