search

capajon / r6maps

Rainbow Six Siege map quick references
MIT License
78 stars 52 forks source link

Add Toggle labels button #139

Closed caseymh closed 5 years ago

caseymh commented 5 years ago

Add a button that allows you to toggle the visibility of labels on the map

purechaose commented 5 years ago

It's been awhile but I finally have some time to look at this. I have a few questions:

  1. Why did you decide to not change the background of the toggle button on focus?
  2. Is there a reason you don't also prevent the background from changing on hover? (in practice, this only allows the button to change color the first time it's clicked, so it looks like it's a bug).
  3. Why are you putting DisplayNone as the default twice (one via data-prevRoomStyle and one via a ternary operator in setupToggleClickEvent)?

Other than that, the only problems I can find are from the original implementation for room styles.

caseymh commented 5 years ago
  1. I didn't change the background on focus because when I did it would change to blue and then it would always stay blue. Looking at it closer it stays blue until you click another button but if you click somewhere on the map it doesn't change back to white. As it is a toggle button I didn't think it should stay highlighted after you click it.
  2. I didn't prevent the background from changing on hover so that it would be more uniform with the other buttons, since they change when hovered over. There is a bug here as if you are hovering and it is in focus it won't change color but if you focus on something else it will.
  3. I think I defaulted it twice because I did the HTML part first and then when I was doing setupToggleClickEvent I put it there to be more robust. I can take it out of the HTML element if you want.