Marble Maps design revisions
Needs ReviewPublic

Authored by juditbartha on Aug 21 2017, 12:32 PM.


Group Reviewers
Marble: Material Maps

Added Options menu as Kirigami ScrollablePage to side panel.

Diff Detail

R34 Marble
Lint Skipped
Unit Tests Skipped
juditbartha created this revision.Aug 21 2017, 12:32 PM

Added Bookmarks as Kirigami ScrollablePage to side panel. Added drag icon to qrc.

Added Drag&Drop functionality to Bookmarks.

Added swipe-to-remove functionality to Bookmarks, but certain bugs persist:
Interrupting a swipe leaves the dark overlay over the unremoved bookmark element.
The dark overlay isn't vertically centered on the bookmark line and looks odd.

rahn added a subscriber: rahn.Aug 21 2017, 5:19 PM

Looks nice to me in general.


I know it's not directly part of this patchset, but I'd still like to mention it:
It's relatively unusual to have visual items inside a MouseArea and semantically it's pretty odd: The object hierarchy should also always reflect the semantics in order to keep good modularity: The delegate here is not primarily an "extended MouseArea" so the MouseArea should not be the root item. You could either put the MouseArea as a child inside the SwipeDelegate (but I guess input handling will interfere then) OR you add it as a sibling next to the SwipeDelegate.

juditbartha updated this revision to Diff 18512.EditedAug 21 2017, 6:14 PM

Added bottom menu containing context-based buttons. When app is in "place" mode the bookmark button (filled / unfilled star.svg) is blurry, even though the Image has the "smooth" attribute, and the svg itself is, of course normal. This doesn't happen with the "route" button, which is used in the same way.
Also positioned Search bar to fill up entire top space, and added "Routing" menu to side panel.

rahn added a comment.Aug 21 2017, 6:28 PM

Looks nice to me in general.

28 ↗(On Diff #18512)

If you don't need the rectangle as such anymore please use an Item instead :)

Fixed Bookmarks swipe-removal color bug, added Kirigami OverlaySheet for "Part of # routes" menu.

Sounds exciting! I'm trying to apply the patches one after the other, but 18505 and 18593 fail to apply. I'm doing

for patch in 18485 18486 18493 18505 18512 18593
  wget "${patch}&download=true" -q -O - | git apply || echo "Patch ${patch} failed :-/"

and get the output as shown in - any idea what's going wrong? 18593 is likely a follow-up failure of 18505 not applying, so 18505 is the one that is somewhat wrong.

Works great, I like it :-)

The side panel seems better, the dialogs look nicer. Some things I noticed that could be improved:

  • I'd change the order "About | Bookmarks | Layer Options | Routing" in the side panel to "Routing | Bookmarks | Layer Options | About"
  • The Accessibility option is still mostly a stub (for now just includes accessibility information in place descriptions, e.g. wheelchair entries. Later on accessibility related changes to the map should be done when it's activated though), so I think it could be moved to the Layer Options menu, what do you think?
  • Settings as the title of the side panel seems a bit odd to me. "Marble Maps" isn't much better either though. Ideas for a different header?
  • The Back key mostly just exits the app
  • The Page (?) animation when leaving e.g. the Layer Options dialog has a strange slow ending here, is that intended?
  • The Bookmarks dialog needs a bit more love :-)

The first three points should be quick to change, the other ones probably need more work / research?