GNOME Bugzilla – Bug 610385
Places unmount icon position
Last modified: 2010-03-29 22:30:35 UTC
Created attachment 154157 [details] Screenshot It is nice that you can unmount partitions from the places section in overview but I think it's not 100% clear where the unmount icon belongs to. In the screenshot I attached, for example, the unmount icon is right next to the "Photos" folder, yet it belongs to "Music". Having a box being drawn around the whole item when hovering would certainly improve things to make sure where the unmount belongs to. Maybe only show the unmount icon on hover to not give false expectations when while hovering.
*** Bug 609573 has been marked as a duplicate of this bug. ***
Created attachment 156653 [details] [review] [placesDisplay] Replace remaining Big.Boxes with St.Widgets While most of the code already is CSS stylable, the two-colum setup is still done using Big.Box with hard coded spacings. Port those remaining parts to St.Widget, so that all spacings can be adjusted by the theme. This patch is not really required by the two following patches, but IMO it's still a nice cleanup ...
Created attachment 156654 [details] [review] [docDisplay] Port DashDocDisplayItem to CSS Replace Big.Box with St.Clickable similar to the code in placesDisplay.
Created attachment 156655 [details] [review] Adjust theme of recent-items and places sections Add a border to items which highlights on hover, just like the style of (non-running) app-well items. For removable items in the places section, this has the additional benefit of making clear to which item the unmount button belongs.
Comment on attachment 156653 [details] [review] [placesDisplay] Replace remaining Big.Boxes with St.Widgets >-const Big = imports.gi.Big; Yay! :) >+ this.actor = new St.Table({ style_class: 'places-section', >+ homogeneous: true }); >+ this._leftBox = new St.BoxLayout({ vertical: true }); >+ this.actor.add(this._leftBox, { row: 0, col: 0 }); >+ this._rightBox = new St.BoxLayout({ vertical: true }); >+ this.actor.add(this._rightBox, { row: 0, col: 1 }); But if we're going to do this, we ought to do it right; we should put the items directly into the table, rather than using a series of boxes to simulate a table. (It looks as though the patch changes the layout as well; previously there was a 4 pixel gap between _leftBox and _rightBox, now it seems that there would only be 2 pixels?)
Comment on attachment 156654 [details] [review] [docDisplay] Port DashDocDisplayItem to CSS >Replace Big.Box with St.Clickable similar to the code in placesDisplay. > >https://bugzilla.gnome.org/show_bug.cgi?id=610385 need to clarify that this actually changes the padding/spacing as well >+ let bin = new St.Bin({ child: this._icon }); >+ box.add(bin); I don't think this is necessary. It looks like the extra box before was to fix up the icon's alignment, but you should be able to do that with StBoxLayout child properties now if it's not right by default. >+ if (button.pressed && this._dragStartX != null) { "pressed" needs to be "held" now, qv a8fa8a49 Though we probably ought to move the St.Clickable special-handling into dnd.js, rather than duplicating it in three different files.
Created attachment 157240 [details] [review] [dnd] Special-case St.Clickable (In reply to comment #6) > Though we probably ought to move the St.Clickable special-handling into dnd.js, > rather than duplicating it in three different files. Makes sense.
Created attachment 157241 [details] [review] [appDisplay,placeDisplay] Remove manual dnd mode As the special handling for St.Clickable moved into dnd, it is no longer necessary to use manual mode in these cases.
Created attachment 157242 [details] [review] [placesDisplay] Replace remaining Big.Boxes with St.Widgets Updated according to review.
Created attachment 157243 [details] [review] [docDisplay] Port DashDocDisplayItem to CSS Updated according to review.
Review of attachment 157242 [details] [review]: Looks great!
Comment on attachment 157242 [details] [review] [placesDisplay] Replace remaining Big.Boxes with St.Widgets Attachment 157242 [details] pushed as 095e15f - [placesDisplay] Replace remaining Big.Boxes with St.Widgets
Created attachment 157311 [details] no spacing for removable device and not shown custom bookmark after this patch ( as you can see on attach) there's no space for removable device and i can't see my custom bookmark
Created attachment 157317 [details] [review] [placesDisplay] Add spacing between actions and devices (In reply to comment #13) > after this patch ( as you can see on attach) there's no space for removable > device Ooops - sorry
Review of attachment 156655 [details] [review]: Yep.
Review of attachment 157240 [details] [review]: Two minor comments. I see why we need to do this, it's ugly though =/ At some point we need to look at pushing dnd down into St. ::: js/ui/dnd.js @@ +74,3 @@ + if (this.actor instanceof St.Clickable) + // internal state, so we start the drag manually on hover change + // special case St.Clickable: grabbing the pointer would mess up the Would prefer calling the function "onStClickableHoverChanged" to make clear it is only used in this special case. @@ +88,3 @@ + if (!hover) { + let hover = button.hover; + _onHoverChanged: function(button) { I tend to prefer using returns to guard instead of nesting, i.e.: if (hover || !button.held) return; (but this is up to you)
Review of attachment 157241 [details] [review]: This ends up being a really nice cleanup.
Review of attachment 157243 [details] [review]: Looks great.
Review of attachment 157317 [details] [review]: Yep
(In reply to comment #18) > I see why we need to do this, it's ugly though =/ At some > point we need to look at pushing dnd down into St. Yes, Owen already suggested this in another bug ... Attachment 156655 [details] pushed as b9e5894 - Adjust theme of recent-items and places sections Attachment 157240 [details] pushed as 04200a4 - [dnd] Special-case St.Clickable Attachment 157241 [details] pushed as feaaefd - [appDisplay,placeDisplay] Remove manual dnd mode Attachment 157243 [details] pushed as df8b033 - [docDisplay] Port DashDocDisplayItem to CSS Attachment 157317 [details] pushed as 73ab59f - [placesDisplay] Add spacing between actions and devices
Created attachment 157420 [details] broken Places section The spacing appears to be fairly broken with these commits. As you can see I have overlap between two elements ("Connect to..." and a File System/Device) and overlap between shortcuts/bookmarks and the Recent section.