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

Fix Sidebar Gap #9376

Merged
merged 2 commits into from
Oct 6, 2014
Merged

Fix Sidebar Gap #9376

merged 2 commits into from
Oct 6, 2014

Conversation

marcelgerber
Copy link
Contributor

This seems to fix #9248, but I'm not quite sure what that code was for (FWIW, it was introduced in 04848ee) and if anything relies on it.

I've tested an verified these scenarios are still working:

  • Invoking menus (context menu (Working Set and file tree), Split View Menu, Working Set sort)
  • Expanding/collapsing folders
  • Resizing the sidebar (via dragging) works fine
  • Hiding/showing the sidebar (via Ctrl-Alt-H) works fine
  • Hiding/showing the sidebar (via double-clicking) works fine
  • Hiding the sidebar by dragging all the way to the edge
  • Showing the sidebar by dragging to the right
  • Showing/hiding/resizing bottom panels
  • Resizing the Brackets window
  • Adding/Removing files to/from Working Set
  • Switching between Split View states (invoking, exiting)
  • Dragging files between Working Sets
  • Window-wide sidebar (+ window resizing)

Not (completely) working:

@RaymondLim
Copy link
Contributor

Thanks @marcelgerber. I confirmed that this indeed fixes the UI glitches.

@redmunds
Copy link
Contributor

This is a pretty big change, so be sure to test all ways that sidebar can be resized. Also test #3376.

@marcelgerber
Copy link
Contributor Author

I just verified:

  • UI jiggles when opening context menu on selection #3376 is UTR, but the Editor itself jiggles (without persistent effects on the UI, just a single jiggle) (doesn't happen all the time)
  • Resizing the sidebar (via dragging) works fine
  • Hiding/showing the sidebar (via Ctrl-Alt-H) works fine
  • Hiding/showing the sidebar (via double-clicking) works fine

@redmunds
Copy link
Contributor

@marcelgerber You can also hide/show sidebar with mouse. Hide by dragging all the way to the left, the Show by grabbing left edge.

Also test resizing/hide/show bottom panels and Brackets editor itself, which also causes sidebar to resize.

@marcelgerber
Copy link
Contributor Author

Tested:

  • Hiding the sidebar by dragging all the way to the edge
  • Showing the sidebar by dragging to the right
  • Showing/hiding/resizing panels
  • Resizing the Brackets window

@redmunds
Copy link
Contributor

Thought of another case: causing working set area to change size by adding/removing files, switching between turning split view on/off, etc.

Can you make a single list of all tests in description at top?

@marcelgerber
Copy link
Contributor Author

I've listed them all in the description.
About #3376: While it's still not completely fixed, imho the behaviour in the PR branch is better as it doesn't cause persistent UI glitches. It's still a weird bug...

@redmunds
Copy link
Contributor

I can't reproduce #9248 in master so I'm not sure if it's fixed here, but this branch seems to fix #3376 for me.

@redmunds redmunds self-assigned this Oct 1, 2014
@peterflynn
Copy link
Member

Fwiw, it looks like the position: relative originally came in with 65c9864, and 04848ee was really just a selector rename. That initial commit was part of the original implementation of the "selection triangle" (#702), so we should test the positioning of that to make sure this doesn't break anything. (I wouldn't expect relative->absolute to cause any trouble there, but just in case...)

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2014

@marcelgerber Great job tracking down the cause of this!

The "selection triangle" is now gone. Aren't we also getting rid of "selection rectangle over the scrollbar"? If so, then we should be able to remove position: relative (as opposed to changing it to position: absolute).

@marcelgerber
Copy link
Contributor Author

The remaining selection "rectangle" works fine, too.
@redmunds Yeah, simply removing position: relative works, too. I'm not sure what's better though as setting it explicitly may prevent future changes that re-introduce that bug.

@peterflynn
Copy link
Member

@redmunds @marcelgerber Just to clarify: the "selection triangle" is still present and isn't going away -- it's just shaped like a square now instead of a triangle. But it still needs to get positioned correctly.

I think removing position: relative works only because that also sets it to position: absolute -- by falling back to the redundant CSS style in the .main-view .sidebar rule. If you remove both of those such that the computed style really is position: static, I wouldn't be surprised if it breaks the positioning of the selection triangle/square.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2014

@peterflynn The "selection triangle/rectangle" in working set has a positioning base ancestor of ".open-files-container", so it's not affected by this change.

The "selection triangle/rectangle" in project tree has a positioning base ancestor of "#project-files-container", so it's also not affected by this change. Note this is currently broken and being tracked in #9203.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2014

@marcelgerber I think the fix is to explicitly set it to position: static. I've played around with that and it seems to work the same as position: absolute.

@marcelgerber
Copy link
Contributor Author

If both work the same, what's the reason I should change it?

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2014

@marcelgerber position: static is the default and simplest positioning. I don't see any reason for using position: absolute unless element needs to be repositioned or used as a positioning base.

@peterflynn
Copy link
Member

@redmunds Instead of doing that, why don't we just remove the two position attributes from the #sidebar and .sidebar selectors? It has the same effect, plus the benefit of removing dead code from our CSS.

@redmunds
Copy link
Contributor

redmunds commented Oct 1, 2014

@peterflynn I'm fine with that after verifying that they're not used by any other elements. I don't like the idea of having an id and a class with the same name, so that would be nice to clean up.

@marcelgerber
Copy link
Contributor Author

@redmunds Actually, position: static doesn't work for me as it doesn't fix the bug and places the sidebar icons (Configure Working Set, Split View) oddly.
I still refactored the CSS, though.

@redmunds
Copy link
Contributor

redmunds commented Oct 6, 2014

@marcelgerber Good catch on the sidebar icons -- I didn't notice that.

I verified that there is no #sidebar selector (only #sidebar-resizer).

Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI glitches after quick open invocation
4 participants