mart added inline comments. INLINE COMMENTS
> tst_pagerouter.qml:56 > + id: router > + initialRoute: "/home" > + columnView: root.columnView I find all the "/" in the examples confusing, not sure is a good naming convention (and works prefectly without having that /, and i don't necessarly care what the convention in other frameworks is). in the hend you can't push directly something like "/home/login" as a single string so doesn't really make sense as paths, i would prefer examples had simple strings as route names, so your route would be indicated as ["home", "login"] > pagerouter.cpp:214 > + > +void PageRouter::navigateToRoute(QJSValue route) > +{ I wouldn't expect a verb like navigate to be always destructive. if the route happens to be the same, then the function should be a no-op if the current route is home/foo/bar and i want to navigate to home/foo/baz i would expect to remove and destroy only bar, keeping the first two pages if the new route is foo/bar/baz, should just push the new one keeping all is there > pagerouter.h:180 > + */ > + Q_PROPERTY(ColumnView* columnView MEMBER m_columnView NOTIFY > columnViewChanged) > + I know we are fixed to columnview now and its fine, but i would prefer the property being called pageStack like in applicationwindow (in reality i would like both being called pageview but it's too late) which doesn't assume which it is, if sme day we would like to support stackview or whatever else we wouldn't have a sore point in the api also, to the property assigning the pagerow (which will look for the columnview internally, but not exposed trough the api) > pagerouter.h:357 > +QML_DECLARE_TYPEINFO(PageRouter, QML_HAS_ATTACHED_PROPERTIES) > \ No newline at end of file newlines! REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D28383 To: cblack, #kirigami, mart, davidedmundson Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart