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

Implement general-purpose tooltip "(?)"-style #5540

Merged
merged 2 commits into from Nov 7, 2017

Conversation

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Nov 7, 2017

e.g.

2017-11-07-141352_247x112_scrot

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Nov 7, 2017

Hum. We already had mx_RoomTooltip as a generic tooltip mechanism (despite the misleading name), so i'm rather surprised that we've created an entirely new mx_Tooltip here which has entirely different CSS (and colouring and aesthetics which is now inconsistent with the existing design). I would have expected instead to have fixed the naming on mx_RoomTooltip and extended it to support multiline descriptions (if it doesn't already). The situation is made even messier by the fact that the dispatcher already has a generic view_tooltip action (hooked up to RoomTooltip). Please yell if i am missing some consideration though?

I'd rather we didn't burn further time on a P2 non-release-blocker thing when we are trying to rush to get a release out the door, so I suggest we just merge it for now and unify all the tooltips down the road. I'm not really sure how to suggest how to avoid this sort of misstep in future, other than trying to keep DRY and maintainability in mind, and being aware that whenever we are adding entirely new CSS for visuals which effectively already exist in the UI, we're almost certainly doing it wrong.

In terms of the code specifics it looks great, and I suggest we merge it for now.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Nov 7, 2017

@lukebarnard1

This comment has been minimized.

Copy link
Contributor Author

lukebarnard1 commented Nov 7, 2017

Yep looks like I've found a particularly bad part of the tradeoff between speed of impl. and maintainability. Sorry about that. I anticipated the modifying of the existing RoomTooltip to be more work than wrapping it and giving it a "?".

The situation is made even messier by the fact that the dispatcher already has a generic view_tooltip action (hooked up to RoomTooltip)

This is handled by the RoomList and doesn't seem to do much other than something specific to the RoomList.

I'd rather we didn't burn further time on a P2 non-release-blocker thing when we are trying to rush to get a release out the door, so I suggest we just merge it for now and unify all the tooltips down the road

sgtm! My bad for trying to half-rush a p2 thing.

maintainability in mind, and being aware that whenever we are adding entirely new CSS for visuals which effectively already exist in the UI

I guess the name mx_RoomTooltip and the way the existing one was already being used lead me to want to create new CSS for it. I'm exactly sure how to handle the CSS for these cases where we wrap an existing thing with something that gives it a more specific purpose.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Nov 7, 2017

turns out the confusion here was actually that Tooltip isn't a generic Tooltip but a generic button which happens to emit RoomTooltips. so we're fine now it's renamed TooltipButton - thanks!

@lukebarnard1 lukebarnard1 merged commit f280fe3 into develop Nov 7, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.