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

Distinguish "Creator" badge on comments #1623

Merged
merged 13 commits into from
Jun 27, 2023
Merged

Distinguish "Creator" badge on comments #1623

merged 13 commits into from
Jun 27, 2023

Conversation

alectrocute
Copy link
Contributor

@alectrocute alectrocute commented Jun 26, 2023 •
edited
Loading

Hi Lemdevs!

In this PR:

  • Make "Post Creator" badge stand out more by:
    - Use "OP" shorthand label by default
    - Add tooltip that says "Creator" upon hover
    - Use .text-info color
Screenshot 2023-06-26 at 4 45 50 PM

This is a really important distinguishing badge on comments. I personally dislike the term "Original Poster", acronym "OP", but it has two advantages here: is shorter, takes up less room on row and is very familiar. So with that said, I agree with #1615. Debate on.

Resolves #1615.

Thanks!

SleeplessOne1917
SleeplessOne1917 approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@SleeplessOne1917 SleeplessOne1917 left a comment

Choose a reason for hiding this comment

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

LGTM, will wait for other feedback before merging. Is the translation for OP merged already?

@alectrocute
Copy link
Contributor Author

LGTM, will wait for other feedback before merging. Is the translation for OP merged already?

Nope, but I'll open a PR!

@alectrocute alectrocute mentioned this pull request Jun 26, 2023
Merged
@noornee noornee mentioned this pull request Jun 26, 2023
Closed
3 tasks
@alectrocute
Copy link
Contributor Author

We’ll also need to make sure the OP/creator badge appears on mobile viewport widths, too.

@SleeplessOne1917
Copy link
Member

We’ll also need to make sure the OP/creator badge appears on mobile viewport widths, too.

There are settings in both firefox and chromium devtools that allow you to mimic the screen size of a mobile device. For example, on firefox:
image

@alectrocute
Copy link
Contributor Author

alectrocute commented Jun 26, 2023 •
edited
Loading

We’ll also need to make sure the OP/creator badge appears on mobile viewport widths, too.

There are settings in both firefox and chromium devtools that allow you to mimic the screen size of a mobile device. For example, on firefox: image

Yep, fully intend on testing it this way. Just leaving a note for myself when I return to this PR later today or tomorrow. 😅

If anybody wants to push to this branch, I won’t stop ya!

@dessalines
Copy link
Member

One thing I do on jerboa (that I nabbed from boost for reddit), is to use a badge on the username itself for post creators:

Screenshot_2023-06-27-07-50-30-008_com jerboa

Up to you, but that's also an option.

@dessalines
Copy link
Member

I merged the translation also, because whatever solution we go with, it could work with tippy.

@alectrocute
Copy link
Contributor Author

@jsit Made your suggested changes!

@dessalines Fixed the issue with it not appearing on mobile.

Screenshot 2023-06-27 at 11 42 47 AM Screenshot 2023-06-27 at 11 42 44 AM

It's lowercase in my screenshots because I forgot to pull the submodules.

We should be good to go!

SleeplessOne1917
SleeplessOne1917 requested changes Jun 27, 2023
View reviewed changes
Copy link
Member

@SleeplessOne1917 SleeplessOne1917 left a comment

Choose a reason for hiding this comment

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

I think the OP badge should be uppercase. It looks like a word and not an acronym when written lowercase.

@alectrocute
Copy link
Contributor Author

I think the OP badge should be uppercase. It looks like a word and not an acronym when written lowercase.

See my latest comment:

It's lowercase in my screenshots because I forgot to pull the submodules.

@alectrocute alectrocute requested a review from jsit June 27, 2023 16:23
jsit
jsit suggested changes Jun 27, 2023
View reviewed changes
src/shared/components/comment/comment-node.tsx Outdated
{I18NextService.i18n.t("creator")}
</div>
<span
className="badge text-info text-bg-light d-sm-inline me-2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need d-sm-inline here anymore

@jsit
Copy link
Contributor

jsit commented Jun 27, 2023

I know this isn't the fault of this PR, but it seems weird that we're hiding the other badges on mobile? That seems like really useful/important info.

@jsit
Copy link
Contributor

jsit commented Jun 27, 2023

I think the OP badge should be uppercase. It looks like a word and not an acronym when written lowercase.

See my latest comment:

It's lowercase in my screenshots because I forgot to pull the submodules.

I did git submodule update and am still seeing lowercase.

@alectrocute
Copy link
Contributor Author

I know this isn't the fault of this PR, but it seems weird that we're hiding the other badges on mobile? That seems like really useful/important info.

Yeah, now that I think about it – These badges are really critical. We should really address this immediately by removing d-none d-sm-inline from all these role badges.

I did git submodule update and am still seeing lowercase.

Weird. I'll just add a .toUpperCase() proto, because what the heck. It's not a big deal I suppose.

@alectrocute
Merge branch 'main' of https://github.com/LemmyNet/lemmy-ui into bugf…
e9f3701 
…ix/make-post-creator-badge-stand-out-more
@alectrocute
Copy link
Contributor Author

Screenshot 2023-06-27 at 1 51 51 PM

@jsit If I show all the role badges on mobile, then it wraps to the next line on comments with all three badges. Thoughts?

@SleeplessOne1917
Copy link
Member

Maybe we could save some space by doing something like what Hexbear does and using letter badges.

Admin:
Screenshot_2023-06-27-13-55-45-60_50ecd26a21c2c56ce608de5e94733463

Mod:
Screenshot_2023-06-27-13-57-51-48_50ecd26a21c2c56ce608de5e94733463

@alectrocute
Copy link
Contributor Author

@SleeplessOne1917 @jsit Please see my latest changes to this branch. I refactored the whole thing. 😅

I like the idea of using letters to save space. I say let's do this! @jsit thoughts?

@alectrocute
Copy link
Contributor Author

Pushed yet another refactor. Let me know what you all think!

@alectrocute
Copy link
Contributor Author

Walkthrough video of most recent changes:

Untitled.webm

@alectrocute
Copy link
Contributor Author

Screenshot 2023-06-27 at 3 02 42 PM

Most recent changes, as per chat on matrix.

@SleeplessOne1917 SleeplessOne1917 merged commit 51a80a8 into main Jun 27, 2023
@SleeplessOne1917 SleeplessOne1917 deleted the bugfix/make-post-creator-badge-stand-out-more branch June 27, 2023 19:10
@alectrocute alectrocute mentioned this pull request Jun 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SleeplessOne1917 SleeplessOne1917 SleeplessOne1917 approved these changes

@dessalines dessalines dessalines approved these changes

@jsit jsit jsit approved these changes

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

Successfully merging this pull request may close these issues.

An OP indicator badge in comment
4 participants
@alectrocute @SleeplessOne1917 @dessalines @jsit