Fix auto-resize for Validate and Explore page #3649
Conversation
|
Interesting... I'm not sure what's different about having the dev console either closed or in a separate window, but in that case the resizing doesn't seem to be working as expected! Looks like it worked like your video when I had the dev console docked to the side like you did though. Peek.2024-09-11.10-28.mp4 |
|
Should be good now! I previously thought that the auto-resize was only supposed to be applied horizontally, not vertically. My bad! |
|
Added a listener that passes through iframe URL changes to the parent document! |
There was a problem hiding this comment.
I'm adding some comments to the code so that we can get it pretty clean before moving on to adding this to the Validate page!
I only managed to find one more issue: there is code in the navbar that references the (formerly global) svl object that is now hidden within the iframe. For example, I get some errors to the console if I sign up for a new account through the navbar on the Explore page now.
Since I haven't really been able to break this in any other fundamental way, I think that we're ready to move on to adding this to the Validate page as well! Would you agree?
This should be fixed now. The Also fixed the other issues you mentioned in the review.
Agreed! I'll be doing that in my next set of commits. |
|
And it's now included in the validate page!! (Ready for review) Edit: Also added to new validate beta |
…m/ProjectSidewalk/SidewalkWebpage into 3626-auto-resize-explore-validate
…Webpage into 3626-auto-resize-explore-validate
|
|
||
| // Get the width and height of the content inside the iframe. | ||
| const contentWidth = contentElement.clientWidth; | ||
| const contentHeight = contentElement.clientHeight + 60; // Add 100px for extra padding. |
There was a problem hiding this comment.
Hmm, if I remove the +100 then part of the UI is cut off on the bottom. Not a big deal because we can play around with the values to get what we want, but ideally the math here would work as we expect!
I assume that part of this has to do with how we are adding padding-top to the body element? But not totally sure. Would be nice to have the padding-top make some sense, as I'm not really sure how it works or how it is supposed to right now!
I think that in an ideal world, we would be able to specify what percentage of the area below the navbar is to be used as white space both above and below the UI... It's not really that important to be able to do this, but whenever I'm fully invested in a section of the code like this, I like to try to refactor a bit to make things make more sense for the future. So if it doesn't feel like a huge lift, any movement in that direction would be great to see!
There was a problem hiding this comment.
Agreed, I made those changes now!
|
My latest commit features the ability to specify vertical whitespace/padding through a global constant called |
|
Previous comment here (now deleted) stated that this was not ready to merge because it had not been added to validate page. Completely forgot that this PR actually had in fact been added to validate page. Sorry about that! So yes, this PR is ready to merge once we review and test. |
There was a problem hiding this comment.
The padding looks different on each page even though the percentage is the same, because we have hard-coded top values for the top-most elements on each page that we've used to kinda make something that feels right in terms of padding. I realize that the "right" way to do this would be to get rid of that extra hard-coded "padding", but that has knock on effects; things like the mission complete screens use take into account the hard-coded number. So to fix this the right way, you'd have to go through and check in on all of our different screens that show up to adjust them after making a change. That's probably just not the best use of our time tbh. So maybe we just do the quick fix of having a PERCENT_VERTICAL_PADDING that is configured by page?
Added two other comments in the code. We're almost done here!
|
Does my most recent commit fix things or am I misinterpreting the issue? Before:
After:
|
|
Ahh, a big piece of the issue is that the NewValidateBeta page has way more space along the bottom as well! |
|
Maybe we can just get rid of this space? I'll try it out - hoping it doesn't break things. |
|
@misaugstad Removing the bottom space on newValidateBeta yields this:
It feels kinda weird because of how wide it becomes. Thoughts? |
|
I like it at first glance! Maybe once I start testing it out it'll feel too big. But then we can just increase the padding percentage! I feel like it looks weird when the padding at the bottom looks like it's 3x as big as the top padding |
|
One thing to also consider (which is only what some related to this
thread): can or should we reduce the size of the menu bat header when in a
task (or even eliminate it all together?)
Sent from phone
…On Mon, Nov 25, 2024 at 6:56 PM Michael Saugstad ***@***.***> wrote:
I like it at first glance! Maybe once I start testing it out it'll feel
too big. But then we can just increase the padding percentage! I feel like
it looks weird when the padding at the bottom looks like it's 3x as big as
the top padding
—
Reply to this email directly, view it on GitHub
<#3649 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAML55OBYUDOXN45Y5RWZDD2CPPOVAVCNFSM6AAAAABN3ZQSJ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJZGU2DKMJXGE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Interesting idea to fully remove it 🤔 I think I like what you had initially proposed in #3504 where we have a smaller version unless we're on the landing page. Or really it's just that it's smaller just in the tools... Not sure |
|
I think I improved the situation with newValidateBeta a bit but it still might not be perfect. When you get the chance, let me know what you think. Screenshots: |
|
@jsomeara okay, ready to merge! Just made a couple small changes, like:
|
Resolves #3626
This PR fixes the auto-resize functionality that was recently broken in a chrome update of the
zoomcss property. It achieves this with an iframe that contains a simplified version of the explore or validate page, and then scales the iframe using thetransform: scale()css property. Using an iframe preserves the proper mouse positions of the pano labels. It is compatible with all major browsers I've tested. I anticipate that it'll work with Safari, but currently untested due to me lacking a Mac.Note: This is currently a draft because I have not yet started the validate page. I think it's better to hold off on that until we're happy with the explore page. Using lessons learned from that one page, I can then apply the same technique to the validate page.
Video
auto.resize.explore.page.mp4
Testing instructions
See here
Things to check before submitting the PR