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 feedback comments not being expandable #1506

Merged
merged 2 commits into from Jun 1, 2022

Conversation

david-venhoff
Copy link
Member

Short description

This pr fixes a bug where feedback comments were not expandable in some cases. This bug existed because the responsible event handlers were (depending on page load order) set up on the <i> elements, which got later replaced by <svg> elements.

Proposed changes

  • Make sure that the image elements are already converted when setting up the event handlers

Resolved issues

Fixes: #1503

I also noticed that the feedback text only gets collapsed when it has more than one line or when it has more than 20 words.
I don't know if this is a problem in practice, but a text with few but long words would break the layout:
grafik

@david-venhoff david-venhoff requested a review from a team as a code owner May 27, 2022 16:08
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 👍

This fixes the issue, however I think there might be a more efficient solution...
Also, could you add a note to the changelog?

I also noticed that the feedback text only gets collapsed when it has more than one line or when it has more than 20 words. I don't know if this is a problem in practice, but a text with few but long words would break the layout

Good point, maybe it makes sense to include this block as last elif case before printing the whole comment?

{% elif feedback.comment|length > 120 %}
    <span>
        {{ feedback.comment|truncatechars:120 }} <i data-feather="chevron-down"
        class="toggle-feedback-comment transform cursor-pointer hover:scale-125"></i>
    </span>
    <span class="hidden">
        {{ feedback.comment }} <i data-feather="chevron-up"
        class="toggle-feedback-comment transform cursor-pointer hover:scale-125"></i>
    </span>
{% else %}
    {{ feedback.comment }}
{% endif %}

@david-venhoff david-venhoff force-pushed the bugfix/fix_feedback_expansion branch from 04062c6 to 03eec1e Compare May 30, 2022 10:31
@david-venhoff
Copy link
Member Author

I have now removed the call to feather.replace and added your suggestions for truncating too long feedback. On my screen the limit of 120 character was quite high, so I went for 60 instead.

@david-venhoff david-venhoff force-pushed the bugfix/fix_feedback_expansion branch from 03eec1e to 22c5f69 Compare May 31, 2022 13:19
@codeclimate
Copy link

codeclimate bot commented May 31, 2022

Code Climate has analyzed commit e59330c and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 73.9% (0.0% change).

View more on Code Climate.

@david-venhoff david-venhoff force-pushed the bugfix/fix_feedback_expansion branch from 22c5f69 to 1c078e8 Compare May 31, 2022 14:08
@timobrembeck
Copy link
Member

@david-venhoff I think my initial solution with truncating the feedback in the django template doesn't work well for all edge cases (e.g. comments with multiple lines that contain very long words or less than 20 words but very long ones etc.), so I tried to do the truncation in css + js...
The solution was a bit too complex for regular GitHub-suggestions, so I directly pushed a commit. Could you have a look at it and check if everything works for you and whether you're ok with my changes? Thanks a lot!

@david-venhoff david-venhoff force-pushed the bugfix/fix_feedback_expansion branch from c6913c2 to a6a7837 Compare June 1, 2022 14:42
@david-venhoff
Copy link
Member Author

Looks good to me. 👍

david-venhoff and others added 2 commits June 1, 2022 19:32
@timobrembeck timobrembeck force-pushed the bugfix/fix_feedback_expansion branch from a6a7837 to e59330c Compare June 1, 2022 17:33
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

🎉

@timobrembeck timobrembeck merged commit 7858ab8 into develop Jun 1, 2022
@timobrembeck timobrembeck deleted the bugfix/fix_feedback_expansion branch June 1, 2022 17:53
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.

Expanding feedback not working
2 participants