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

Make it possible to identify the page author's comments #844

Merged
merged 6 commits into from
May 7, 2022

Conversation

BBaoVanC
Copy link
Contributor

@BBaoVanC BBaoVanC commented Apr 24, 2022

Fixes #82

TODO

  • Documentation
  • CHANGES.rst entry
  • Possibly remove the colors and instead explain how to change them in the docs
  • Screenshot using default Isso theme

Summary of changes

  • Page author's comments will have a different background
  • Their name will have a different color
  • Configured using data-isso-page-author-hashes field in the <script> tag; this can include multiple hashes.
  • Add matching CSS rules, and change the margin/padding rules a bit to make .isso-text-wrapper take up the comment's size
  • Move .isso-follow-up div outside of .isso-text-wrapper

Screenshot at the bottom of this description.

Moving .isso-follow-up

I moved the .isso-follow-up div to be outside of .isso-text-wrapper (so it is instead a direct child of .isso-comment). Then I gave the background color to the .isso-text-wrapper. Without this change, if someone else replies to a comment by the page author, their comment still has the background. This is because the .isso-text-wrapper div would be as big as the parent comment plus the follow-up ones, and the follow-up comments have a transparent background (instead of explicitly setting one).

I couldn't think of any better solution.

Luckily this change only required changing a couple lines in isso.js so it would properly match the new location.

Configuring

Configure by setting the data-isso-page-author-hashes variable to the hash of the user (or a list of them) that should be highlighted. If providing multiple, separate them with a comma, space, or both.

For example, these all work (I made up the hashes though):

  • data-isso-page-author-hashes="86g7n8g67nm,8m787mg8"
  • data-isso-page-author-hashes="86g7n8g67nm 8m787mg8"
  • data-isso-page-author-hashes="86g7n8g67nm, 8m787mg8"

This is parsed using code from this article.

Screenshot with the theming shown in docs:

Screenshot 2022-05-07 at 14-20-11 Isso Demo

.isso-comment.isso-is-page-author > .isso-text-wrapper {
    background-color: #bae0ea;
}

.isso-comment.isso-is-page-author > .isso-text-wrapper > .isso-comment-header > .isso-author {
    color: #19798d;
}

@BBaoVanC
Copy link
Contributor Author

I'm trying to think of the best way to handle it. One way could be to just make it the job of the website owner to add their own CSS that styles the element based on the data-hash field. Although that would pretty much limit it just to changing colors/backgrounds/etc. Ideally there should be a badge of some sort, probably something server side. Maybe a field in the client named data-isso-page-owner or something.

@ix5
Copy link
Member

ix5 commented Apr 24, 2022

Related: #321, a previous attempt, sadly abandoned by the original author.

@ix5 ix5 added client (Javascript) client code and CSS improvement Not a new feature, but makes Isso more pleasant to use labels Apr 24, 2022
@ix5 ix5 added this to the 0.13 milestone Apr 24, 2022
@BBaoVanC BBaoVanC marked this pull request as ready for review April 24, 2022 03:58
@ix5
Copy link
Member

ix5 commented Apr 24, 2022

Hi, thanks for your work. I'll review the merits of this later, just in the meantime, please try to keep the level of messages down a bit. You can edit your original message by clicking the three-dot button on the top right corner instead of writing a stream-of-consciousness barrage, which will make it quite hard for people to quickly decipher the core points later on.

Your commits should be squashed (or at least split into logical units) and should also mention the name of the component they're touching. See #805 (review) or #831 (comment)

@BBaoVanC BBaoVanC force-pushed the author-badge branch 2 times, most recently from da4a0a4 to 0b64ba1 Compare April 24, 2022 19:39
@BBaoVanC
Copy link
Contributor Author

Hi, thanks for your work. I'll review the merits of this later, just in the meantime, please try to keep the level of messages down a bit. You can edit your original message by clicking the three-dot button on the top right corner instead of writing a stream-of-consciousness barrage, which will make it quite hard for people to quickly decipher the core points later on.

Your commits should be squashed (or at least split into logical units) and should also mention the name of the component they're touching. See #805 (review) or #831 (comment)

Ok, I fixed that now, check the main description for the pull request.

Also is this commit message okay?

isso: Add page-author-hash option to highlight page author's comments

- Give page author's name a different color
- Give the comment a different background
- Supports multiple page authors
- Moved `.isso-follow-up` outside of `.isso-text-wrapper`

I feel like it would be weird to split the CSS and JS changes apart since they both require eachother for the feature to work correctly.

@ix5
Copy link
Member

ix5 commented Apr 26, 2022

Re: commits and splitting:

Ok, I fixed that now, check the main description for the pull request.

Also is this commit message okay?

isso: Add page-author-hash option to highlight page author's comments

- Give page author's name a different color
- Give the comment a different background
- Supports multiple page authors
- Moved `.isso-follow-up` outside of `.isso-text-wrapper`

I feel like it would be weird to split the CSS and JS changes apart since they both require eachother for the feature to work correctly.

You can split the commits, most logical would be one commit for each component or for one coherent/atomic change, e.g. the CSS modifications. What I meant is that you need to get rid of something that looks like:

  • Initial commit
  • Oops, fix typo
  • Rework component X
  • Fix test
  • Revert 'Rework component X

This is hard to review. Especially a lot of "Fix X" and then "Fix Y" stuff.

I hope you understand what I mean. For instance, you introduced a is_page_author variable in on of the commits in your previous history and then removed it again.

Just structure the commits like you structured the headlines in the (now edited and much more clear) original post.

Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

Please document the new option extensively and add a well-written CHANGES entry.

Would also be nice to have screenshots of the thing in action with the default colors, not your dark theme.

EDIT: Also, there should be an explanation how to derive the hash and what it means. Also, hash = singular, but you make it so Isso accepts a list. Should be hashes instead then.

isso/css/isso.css Outdated Show resolved Hide resolved
isso/css/isso.css Outdated Show resolved Hide resolved
isso/js/app/config.js Outdated Show resolved Hide resolved
isso/js/app/templates/comment.js Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this again!

Just so that you don't forget: There should be an explanation how to derive the hash and what the hash means.

isso/js/app/templates/comment.js Outdated Show resolved Hide resolved
isso/js/app/isso.js Show resolved Hide resolved
Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

Some small things still to fix, but all in all, looking good.

Also related: #261

isso/js/app/isso.js Show resolved Hide resolved
docs/docs/reference/client-config.rst Show resolved Hide resolved
isso/js/app/templates/comment.js Outdated Show resolved Hide resolved
isso/css/isso.css Show resolved Hide resolved
@ix5 ix5 merged commit 0c86735 into isso-comments:master May 7, 2022
@ix5
Copy link
Member

ix5 commented May 7, 2022

Merged. Thank you for so diligently applying my feedback, @BBaoVanC !

@BBaoVanC BBaoVanC deleted the author-badge branch May 7, 2022 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client (Javascript) client code and CSS improvement Not a new feature, but makes Isso more pleasant to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Identifying the author of the document
2 participants