Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

do not calculate size with hidden elements when resizing #7417

Merged
merged 1 commit into from Apr 4, 2014

Conversation

zaggino
Copy link
Contributor

@zaggino zaggino commented Apr 4, 2014

just a small fix when there's a hidden element (like invisible spinner toggled on demand) which causes $resizableElement to be smaller than it can be (and leaves weird space there)

Before:
image

After:
image

cc @peterflynn @RaymondLim

zaggino added a commit that referenced this pull request Apr 4, 2014
do not calculate size with hidden elements when resizing
@zaggino zaggino merged commit c4116a7 into master Apr 4, 2014
@peterflynn
Copy link
Member

@zaggino I think even for something small like this we'd prefer you wait to give someone a chance to review before merging. Especially since we're nearing the end of a sprint. (We usually only "self-merge" zero-risk changes like README.md edits).

@zaggino
Copy link
Contributor Author

zaggino commented Apr 4, 2014

Ok, sorry - won't do it again. Should I revert?

@peterflynn
Copy link
Member

I think @RaymondLim or I should review it post-facto, but I don't think think we should worry about reverting unless we find some sort of problem...

@zaggino zaggino deleted the zaggino/resizer-fix branch April 4, 2014 23:33
@peterflynn
Copy link
Member

@zaggino Sorry, just got a chance to look at this. I'm confused by what kind of 'hidden' elements require this -- lots of Brackets bottom panels are hidden using display: none (via jQuery .hide()), and they don't mess up the measurements at all. Is this change for visibility: hidden instead, or something? (In which case this would be a little weird since visibility: hidden is explicitly supposed to still take up space in the layout -- it's more like opacity: 0).

@redmunds
Copy link
Contributor

I wonder if this may fix the problem with the sidebar that's been driving us nuts?

@zaggino
Copy link
Contributor Author

zaggino commented Apr 16, 2014

Actual element here is <div class="spinner large"></div> and by looking at the Brackets classes it really uses visibility: hidden; so I'll be fine if this is reverted and I'll override this class in my styles. It didn't occur to me that this would be this case...

@peterflynn
Copy link
Member

@zaggino Yeah, I think it might make sense to back this out since normally visibility: hidden is explicitly supposed to behave the opposite way. No rush though, just avoids confusion.

@redmunds It seems unlikely to affect #3376 since we don't normally have visibility: hidden DOM nodes in the editor area, and the bug still repros even after this PR landed.

zaggino added a commit that referenced this pull request Apr 16, 2014
@redmunds
Copy link
Contributor

@peterflynn FYI, The problem with #3376 seems to be in the sidebar or project tree, not the editor (even though it's triggered from editor).

@peterflynn
Copy link
Member

@redmunds Ok, in that case this seems even less likely to have any impact on that bug, since this code very specifically only applies to the #editor-holder DOM tree.

peterflynn added a commit that referenced this pull request Apr 18, 2014
revert #7417 - visibility:hidden _should_ leave space in the layout, and we already handle display:none properly
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants