HeartofPhos / exile-leveling

Path of Exile Leveling Guide
https://HeartofPhos.github.io/exile-leveling/
159 stars 31 forks source link

style: add min-height to Viewport #61

Closed williamsonm closed 1 year ago

williamsonm commented 1 year ago

The height of the viewport div was zero on Safari, so set a min-value to make the skill tree visible. Tested on MacOS chrome/safari/firefox.

fixes #51

HeartofPhos commented 1 year ago

Does setting flex-grow: 1 instead of min-height fix the issue as well? Would like to avoid setting min-height in multiple places if possible

ennukee commented 1 year ago

Have you tested this on an iOS device @williamsonm? I mimicked your change in my local dev and tested on my phone and am getting crashes when I try to change the tree using the controls under it. Not sure if this is a problem with my own device, or if there's a larger issue (like the transform being a problem on ios safari as mentioned by Heart in the original ticket)

edit: Tested it on my iPad as well, which works and makes me wonder if my iPhone is lacking in processing power or something of that sort.

williamsonm commented 1 year ago

Does setting flex-grow: 1 instead of min-height fix the issue as well? Would like to avoid setting min-height in multiple places if possible

It does not. I don't like hard-coding height values, either, so there's likely a better solution.

Have you tested this on an iOS device @williamsonm?

Seems to work on an iPhone 14 in both Safari and Chrome.

ennukee commented 1 year ago

Some further notes, I was able to get this working by explicitly setting display: flex; on the .viewport style in PassiveTreeViewer, and then replacing height: 100%; with align-self: stretch; where you have your current change. edit: Turns out that second part isn't even needed, I'm able to see things with just the display: flex; and removing height: 100%

However, I am noting that both my iPad and iPhone will crash. iPad less often, but my iPhone is unable to change trees at all. My iPad can change a few times, but will crash if I change too quickly. This happens using with both of our implementations for me.

"A problem repeatedly occurred on {{link to local dev environment}}" is what Safari shows when these crashes occurs. Chrome suffers from the same crash problem though doesn't give me any sort of message,

If you attempt to rapidly change trees using the < and > controls, do you see any sort of lag or crashes? (including page reloading itself)

edit: Also worth noting my iPhone is much older, it is an iPhone 11 on iOS 14.7, which could explain why it can't even change once

HeartofPhos commented 1 year ago

Turns out that second part isn't even needed, I'm able to see things with just the display: flex; and removing height: 100%

This does feel like a cleaner solution if it works across all devices

ennukee commented 1 year ago

OK, I looked into it a bit and I was able to make modifications to reduce the browser load of trying to calculate the matrix3d calculation. I am going to put this in it's own PR for now.

See #63

ennukee commented 1 year ago

With my tweak in #63 and my proposed change of simply adding display: flex; to PassiveTreeViewer/styles.module.css -> .viewport style, then removing height: 100% from Viewport/styles.module.css -> .viewport style, this looks to be working smoothly on both of my iOS mobile devices with no crashes.

@williamsonm would you like to check out my proposed change and confirm if it works and implement it here, or would you prefer me to add it to #63 ?

williamsonm commented 1 year ago

@williamsonm would you like to check out my proposed change and confirm if it works and implement it here, or would you prefer me to add it to #63 ?

If you have a fix for it, then just add it to your PR and mark this one as closed.

HeartofPhos commented 1 year ago

I've update the branch as per @ennukee, appears to be working on Firefox, Edge and Firefox Android. Is Safari behaving as intended with these changes?