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 follow buttons #382

Merged
merged 3 commits into from
Oct 1, 2021
Merged

Conversation

HebaruSan
Copy link
Contributor

@HebaruSan HebaruSan commented Aug 5, 2021

Motivation

I've wanted to rework the very janky follow/unfollow buttons since #343, but that pull request had a lot of other things going on and I did not want to increase its scope. #379 focused exclusively on the follow buttons, and this pull request replaces that one.

Too much of the visual and style info about the follow/unfollow buttons has been encoded into client script code. The click event handler manipulates the text, tooltips, glyphicon classes, etc., to transform a follow button into an unfollow button and vice versa. This essentially means the client script code needs to know everything about how these buttons are supposed to appear, which is hard to maintain and prone to bugs.

Problems

  • Trying to unfollow returns
    500 Internal Server Error: Dependency rule tried to blank-out primary key column 'mod_followers.user_id' on instance '<Following at 0x7f5c90a31190>'
    
  • Clicking follow or unfollow on mod pages adds this weird symbol to the buttons:
    image
  • (Un)following on one of the browse/overview pages using the small star button only toggles the star for that mod box, but not for other mod boxes of the same mod.
  • If a mod appears multiple times on the screen and you click the star to follow it, all copies of it appear to receive the solid star following indicator, but when you hover them, all except the one you clicked appear to reset to unfollowed (empty star)
  • If you click the follow button once, an alert tells you that you will receive emails. If you dismiss this alert and then follow again, you will be taken to the /register page.

Causes

  • With the database changes from Let users choose which emails to receive #369, the code in mods.unfollow() no longer works. It was kinda weird anyways.

  • With the mod box redesign from Improve mod box appearance #343 the code for (un)follow buttons has been changed in a way that assumes it would only be called for the mod box star buttons, however it's still executed for the big (un)follow buttons on the mod pages. It tries to add the (empty) star glyphicon to the button, which fails to render correctly.

  • The JS code tries to select the follow buttons in the mod boxes by id, however the id selector will only ever return up to one element, as more than one element with the same id is invalid HTML: https://api.jquery.com/id-selector/

  • The click handler of the follow button updates the solid star indicator for all copies of a mod in the page, but for everything else it only modifies the element you clicked on

  • Bootstrap removes alerts from the DOM when they are dismissed:
    https://getbootstrap.com/docs/4.0/components/alerts/

    Method Description
    $().alert('close') Closes an alert by removing it from the DOM. If the .fade and .show classes are present on the element, the alert will fade out before it is removed.

    So the next time we try to show that alert, its element is no longer available; an exception is thrown, and the click handler interprets this as the user not being logged in and redirects them to /register.

Changes

  • Now we do a proper query on the mod_followers/Following table, and delete all rows with for that mod and user (hopefully there's only one, but it handles any case fine).
  • All the id attributes in mod-box.html are moved to classes, as none of them would ever be unique on a page. The follow button code queries and toggles the stars based on the class, not id.
    I couldn't find any use of the other modbox-*-* ids in any code, so nothing else should need adjusting.
  • Now the templates render both the follow and unfollow buttons and rely on classes from the stylesheets to show and hide them. When you click one, we replace .not-following-mod with .following-mod or vice versa for all elements that match .mod-{{mod.id}}, which is intended to be any element that depends on following or not-following state. Instead of manipulating the details of the elements' attributes, we're just telling them whether the mod should show a following state, and stylesheet rules take care of what should be shown or hidden. This makes the toggle work properly for all copies of a mod that are in the page, and means that we can freely change how the buttons look in the templates without the client script code missing them up.
  • Now we override the follow alert's close handler to just add the hidden stylesheet class rather than removing it from the DOM (suppressed by returning false). This way it remains available in case we want to show it again.

@HebaruSan HebaruSan added Type: Bug Area: Backend Related to the Python code that runs inside gunicorn Priority: Normal Status: Ready Area: Frontend Related to HTML, JS, CSS, or other browser things labels Aug 5, 2021
@DasSkelett
Copy link
Member

DasSkelett commented Sep 29, 2021

Works mostly fine now, just one unfortunate Bootstrap weirdness:
on a mod page, the "Follow" button is 5 pixels lower than the "Unfollow" button, so it jumps up and down when you click them.
This is because Bootstrap applies a margin-top: 5px to all btn-block that follow another btn-block:
https://github.com/twbs/bootstrap/blob/68b0d231a13201eb14acd3dc84e51543d16e5f7e/less/buttons.less#L156-L159

not sure how to deal with that. Hard-code the margin-top to 0?

@HebaruSan
Copy link
Contributor Author

What about putting this in between them?

<div class="hidden">dummy to stop Bootstrap from adding padding to the second button</div>

@DasSkelett
Copy link
Member

That should work as well. Maybe put the text as Jinja comment ({# ...#}) though, users don't care and it saves a few bytes over the wire?

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

That worked! Should be all set now, thanks for the extensive fixes 👍

@DasSkelett DasSkelett merged commit 865ce04 into KSP-SpaceDock:alpha Oct 1, 2021
@HebaruSan HebaruSan deleted the fix/follow-buttons branch October 1, 2021 18:36
This was referenced Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Backend Related to the Python code that runs inside gunicorn Area: Frontend Related to HTML, JS, CSS, or other browser things Priority: Normal Status: Ready Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants