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

Changes to language tag #1036

Merged
merged 4 commits into from
May 28, 2023
Merged

Changes to language tag #1036

merged 4 commits into from
May 28, 2023

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented May 23, 2023

  • Dont use bright white color because its not that important
  • Hide tag in case of undetermined (doesnt add any information)

Before:
Screenshot_20230523_221859

After:
Screenshot_20230523_221840

Not sure about the specific css classes, we can also change it as long as it's gray like the other text and not bright white.

- Dont use bright white color because its not that important
- Hide tag in case of undetermined (doesnt add any information)
)?.name
}
</span>
{cv.comment.language_id != 0 && (
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: !== instead of !=.

)?.name
}
</span>
{post_view.post.language_id != 0 && (
Copy link
Member

Choose a reason for hiding this comment

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

Same as other comment.

@SleeplessOne1917
Copy link
Member

Regarding the css class badges: you can get a badge of a given color with "badge badge-(color name here)". The color name would be something from the theme, like light, dark, info, primary etc. Text gets colored similarly with class names like "text-(theme color)".

Also, how does this look in lightly?

@Nutomic
Copy link
Member Author

Nutomic commented May 24, 2023

Good suggestion, badge-muted looks decent as its the same color used for other text in that area (like the creation time). I also changed the admin/mod badges to muted (see #1037) and moved the creator badge first.

Screenshot_20230524_122115

Screenshot_20230524_122133

{i18n.t("mod")}
</div>
)}
{isAdmin_ && (
<div className="badge badge-light d-none d-sm-inline mr-2">
<div className="badge badge-muted d-none d-sm-inline mr-2">
Copy link
Member

@dessalines dessalines May 24, 2023

Choose a reason for hiding this comment

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

There is no such thing as badge-muted: https://getbootstrap.com/docs/4.6/components/badge/

badge-light is your best bet for most themes. You might be able to add another muted class to it, like text-muted

Copy link
Member Author

Choose a reason for hiding this comment

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

Youre right it actually has no effect. So I removed badge-muted, only leaving badge and it still looks the same as in the screenshots.

@@ -345,7 +345,7 @@ export class PostListing extends Component<PostListingProps, PostListingState> {
)}
</li>
{post_view.post.language_id !== 0 && (
<span className="mx-1 badge badge-muted">
<span className="mx-1 badge">
Copy link
Member

Choose a reason for hiding this comment

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

You're not supposed to use it without the 2nd badge-x class, as it might look weird on some themes. Did you try badge-secondary ?

Copy link
Member

Choose a reason for hiding this comment

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

Secondary will be orange on lightly I think. It'll bring us back to the badge being too prominent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well its working...

Copy link
Member

Choose a reason for hiding this comment

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

If it looks good on both the litely and darkly, I have no problems merging. Might give issues on other themes tho.

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.

3 participants