[CLOSED] Add inline editor styling

[core-ai-bot]

Issue by jasonsanjose Thursday Mar 01, 2012 at 01:56 GMT Originally opened as https://github.com/adobe/brackets/pull/353

---

Use JS to position filename div. Update filename div's on window resize.

---

jasonsanjose included the following code: https://github.com/adobe/brackets/pull/353/commits

[core-ai-bot]

Comment by tvoliter Thursday Mar 01, 2012 at 20:10 GMT

---

I haven't seen Garth's original design, but I checked out the pull request and here are my impressions:

• The design for the inline editors feels really heavy. The large dark shadows and gray background pull my attention away from the code to inline editors in a really strong way. When there was no visual treatment I felt I could keep the inline editors up and keep coding since they were not intrusive. Now I find them so visually strong I find it a bit hard to code in the main editor with inline editors present. Perhaps light shades of gray and shorter shadows would improve this?
• The inline editors feel like they pop out rather than in. I think the lower shadow needs to be much shorter to visually make the editor go beneath the main editor
• I like having the inline editors have a different background color though I wish the color was an even lighter gray, enough so I see the difference, but subtle enough that the inline editors integrate well with the main editor

[core-ai-bot]

Comment by jasonsanjose Thursday Mar 01, 2012 at 20:27 GMT

---

Here's Garth's design in a separate repo https://github.com/GarthDB/BracketsSandbox.

Note that we're not implementing the dark theme in the host editor.

[core-ai-bot]

Comment by tvoliter Thursday Mar 01, 2012 at 20:52 GMT

---

After talking with Jason I realized that Garth's design is on a black background, but the current implementation is on what. That makes most of my comments not relevant since I was reviewing the build.

I do feel that even with the black background that the gradients back the inline editor appear to pop out rather than in.

[core-ai-bot]

Comment by tvoliter Thursday Mar 01, 2012 at 21:43 GMT

---

Two issues I found testing this:

• On Mac the inline editor is being drawn on top of the scrollbar (not a problem on PC)
• On PC and PC the filename tag for the inline editor is cut off a bit on the right side

[core-ai-bot]

Comment by peterflynn Thursday Mar 01, 2012 at 23:07 GMT

---

Re Ty's last bullet, the problem is that the horizontal position of that tab isn't taking into account the outer editor's vertical scrollbar. If there's no vertical scrollbar, it's not cut off.

@njx, is that a bug in how CodeMirror sets the horizontal size of the inline widget? I guess this is a sadly common issue with width: 100% in all CSS... But it does seem wrong that each inline widget would have detect when its host editor's scrollbars appear and disappear and add/remove extra right padding into its layout to prevent this clipping...

[core-ai-bot]

Comment by tvoliter Thursday Mar 01, 2012 at 23:38 GMT

---

I am done code reviewing this. It sounds like NJ is picking this task up. NJ, do you want to fix these issues first, or should we merge and just log them as bugs?

[core-ai-bot]

Comment by njx Friday Mar 02, 2012 at 02:54 GMT

---

The latest push changes the way the filename tab is managed. The previous method didn't work with horizontal scrolling (since we would want the tab to stay fixed relative to the right edge of the browser window), and changing it to update its position on horizontal scroll made for a janky appearance, because the tab would scroll first before being repositioned.

So I now use position: fixed for the tab to make it fixed relative to the right edge of the browser window, and then update its vertical position whenever the code is scrolled. This removes it from the scroller's layout, making it scroll smoothly as the scroller scrolls. However, in order to achieve this, I had to do some finagling with z-order, and there's still a lingering bug where it overlaps the bottom scrollbar. I think it's better to go ahead and check it in at this point and deal with that issue as a bug, especially since the visual design may change later on to not require a tab.

[core-ai-bot]

Comment by njx Friday Mar 02, 2012 at 02:56 GMT

---

Note that this push requires a change to CodeMirror, from this pull request: https://github.com/adobe/CodeMirror2/pull/30 -- you'll need to grab that branch (nj/scroll-accessor) in CodeMirror as well.

[core-ai-bot]

Comment by njx Friday Mar 02, 2012 at 02:57 GMT

---

Also, in this change I lightened up the shadows and the background a bit. I sent mail to Garth asking him to approve the changes--they're easy to tweak/back out if we need to later.

[core-ai-bot]

Comment by njx Friday Mar 02, 2012 at 06:59 GMT

---

I've reverted my changes to the way the tab is positioned. Going down that path started to uncover a number of issues which would require some amount of architectural refactoring to get right, and I didn't want to try to land that all tonight.

Going back to the original approach causes three main issues: (1) The tab scrolls horizontally with the code, which it shouldn't. (2) As you resize, the tab's repositioning is rather slow (this is probably related to other issues with JS window resize handlers, and not specific to this code). (3) Relying on the DOM node removal event to remove the window resize handler is erroneous because the DOM node is removed whenever the widget goes outside the virtualized scroll area (so if you make a large file, open a widget, scroll it far off the screen, then scroll back, the tab no longer moves on window resizes).

Given that the visual design might change in the future to not have this kind of tab, I'm not inclined to spend any more effort on fixing these issues right now. Will discuss at tomorrow's scrum.

[core-ai-bot]

Comment by tvoliter Friday Mar 02, 2012 at 17:47 GMT

---

reviewing