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

fix: upvote widget not rendered when comment is collapsed #1583

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

aegroto
Copy link

@aegroto aegroto commented Nov 12, 2024

Description

The upvote widget is now hidden when comments are collapsed, making them not zappable. Fixes #1572

Screenshots

master:
master

fixed:
pr

The top comment is not upvotable when collapsed anymore.

Additional Context

As of now the button is simply not rendered. I don't know if it's desirable to add a graphical widget which does nothing but still appears on collapsed comments.

Checklist

Are your changes backwards compatible? Please answer below: Yes, as soon as the feature was not relying on zapping collapsed comments by clicking on the button (I hardly believe so).

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below: 8. I have tested it on a variety of threads, however I have not written any unit test as the test suite for this specific component was not initialized yet.

For frontend changes: Tested on mobile? Please answer below: Yes, everything works correctly.

Did you introduce any new environment variables? If so, call them out explicitly here: No

@huumn
Copy link
Member

huumn commented Nov 12, 2024

Welcome!

We still want the bolt to take up horizontal space - we just need it to not be clickable/make a zap when the comment is collapsed. That way, there isn't horizontal shift when a comment is collapsed/uncollapsed. Does that make sense?

@aegroto
Copy link
Author

aegroto commented Nov 12, 2024

Welcome!

We still want the bolt to take up horizontal space - we just need it to not be clickable/make a zap when the comment is collapsed. That way, there isn't horizontal shift when a comment is collapsed/uncollapsed. Does that make sense?

Thanks for the rapid feedback :) Sure! I will find a solution to keep the layout the same while collapsed and push it as soon as I can.

@aegroto
Copy link
Author

aegroto commented Nov 12, 2024

Done, this is how collapsed comments look right now:

immagine

This new solution should cache the icon, making it faster to load as well when a comment is un-collapsed (I haven't perceived any difference myself but I am on a desktop and with high bandwidth).

Copy link
Member

@huumn huumn left a comment

Choose a reason for hiding this comment

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

Could we simply set disabled = true rather than adding another condition?

@aegroto
Copy link
Author

aegroto commented Nov 12, 2024

Could we simply set disabled = true rather than adding another condition?

It could be possible to merge these two variables in the styling node (the call to classNames). However, If I have understood the code correctly, disabled means that the user cannot upvote the comment, adding the noSelfTips CSS class. In this case, !visible means that the user may upvote the comment, but the button is not currently visible.

I believe it may be more solid to have two separate variables for these states, but I am not a React expert so if you have any suggestion on how it could be implemented I will fix the PR as soon as I can. Also, I am not sure if it's possible to remove the visible argument from the widget as this option depends on the value of the collapse string in the parent comment component.

@huumn
Copy link
Member

huumn commented Nov 12, 2024

My suggestion would be to set disabled = true when the comment is collapsed if it's possible. If the comment isn't collapsed, leave disabled as it is.

If that's not possible, all good and you can leave it as is.

@aegroto
Copy link
Author

aegroto commented Nov 12, 2024

Sure, I believe it's possible, but I will give a look into it tomorrow and updated the PR as soon as possible.

@aegroto
Copy link
Author

aegroto commented Nov 13, 2024

I have made some modifications to use the disabled condition only when doing the checks for rendering, let me know if you prefer this version or revert to the old one. Unfortunately I didn't find a way to remove the visible argument as item does not contain any information about the comment being collapsed or not. An alternative would be to parse the classNames argument checking for a specific class which indicates the comment is collapsed, but it is slower and I find it to be less intuitive as well.

@ekzyis ekzyis added the bug label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collapsed comments can be zapped
3 participants