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
