Hiding buttons is not working correctly


(Sam Saffron) #1

I just checked in:

The reasoning is the following:

  • My personal edit button is now suppressed incorrectly, despite having ample room for it.

Its adding an enormous amount of friction for no reason. Space is not saved.

  • Keyboard shortcuts are broken, they should expand and then click on the action

I really don’t want to re-enable this until both are fixed. I am concerned about burying bookmark as well.

  • Edit should not be suppressed on wiki.

cc @codinghorror @eviltrout @lightyear


Move some post actions to "extra drawer"
(Jeff Atwood) #2

That is really annoying that keyboard shortcuts got broken, I specifically asked @lightyear about that in his PR in GitHub comments.


(Sam Saffron) #3

I am pretty sure @eviltrout broke it when he re-implemented the feature

https://github.com/discourse/discourse/commit/d06720d059f992999178f3eafc168e1018800644


(Benjamin Kampmann) #4

I agree with you. By making it two lists of “shown” and “hidden” buttons, a button may only be in one. While with the counter I had previously, was counting on actually rendered elements, making it always four elements exactly (+reply) and only show the ellipsis when it would be replacing at least two buttons. The current behaviour here on meta of hiding bookmark behind the ellipsis is kinda awkward, indeed.

But even in this approach, shouldn’t be too hard to fix. If there is only one button, don’t render the ellipsis but the button instead. Regarding the hotkeys, yes, they did work – even on non-visible items. As said, this concern was the main reason I chose the approach of only hiding with JS/css. I don’t know what broke them in the new approach.

May I ask another question though, a more general one. Why, though agreed upon the max_items approach, did we change to the other approach and rewrite the same feature within just a few hours? What were the reasons behind changing to this approach?


(Robin Ward) #5

I got an urgent call from @codinghorror saying the animation had to be removed immediately. Also some buttons were being hidden that shouldn’t have been.

When I looked at the code there was no way to change the order easily as it was based on an index, and some of them would arbitrarily fall behind the index. So I opted for the hidden list and and the non hidden list instead. Additionally I really didn’t like matching the string on the HTML of the button for reply so I ripped that out.

Of course I accidentally broke the keyboard stuff that worked, and the edit button thing was not considered, so I think the lesson here is I probably should have reverted the commit and we should have discussed it again, rather than furiously trying to change it once it was live, since that didn’t work out too well in the end :frowning:


(Jeff Atwood) #6

This is now correct, but number of buttons visible should be 4 + ellipsis so we can show bookmark. Closing in advance.


(Jeff Atwood) #7