Fix fullscreen menu state

Tucsky / aggr

Cryptocurrency trades aggregator

https://charts.aggr.trade/

GNU General Public License v3.0

830 stars 240 forks source link

Fix fullscreen menu state #289

Closed mwyywm closed 1 year ago

mwyywm commented 1 year ago

When pressing esc key to leave fullscreen mode the fullscreen state in the menu component is not updated.

Added event listeners to change the isFullscreen state in menu component.

I tested it and it works on firefox, chrome and safari.

adeacetis commented 1 year ago

Hey,

Apparently we didn't know it was broken. Thank you for proposing this.

However, to move forward, could you please rebase your commit onto master - don't include neither @Tucsky's nor the merge commit - and describe the steps to reproduce what you fixed ? Add screenshots (before/after) if useful.

Thank you.

mwyywm commented 1 year ago

I've rebased the commit now.

describe the steps to reproduce what you fixed

To reproduce:

Go to the live site at https://aggr.trade/
Open the menu at the bottom right of the page.
Click the "Go Fullscreen" button.
Press the ESC key to leave fullscreen mode.

We expect the button to say "Go Fullscreen" but it shows Exit.

Image below shows how it looks like when we leave fullscreen mode on the live site.

This pull request makes the fullscreen button in the menu component to be in the correct state after exiting fullscreen mode.

adeacetis commented 1 year ago

Awesome. Thank you for taking the time to explain this.

Just did a quick test right now, and I see the bug you were talking about, I will test your solution later tonight.

adeacetis commented 1 year ago

Tested on Chrome 113.0.5672.92, Firefox 113.0, Edge 113.0.1774.35

I have noticed a couple of other bugs related to this very label. I have tested production and it's there as well, so unrelated to these changes. I will create a new issue.

Thank you!