1232 redesigned top level structure for the settings #1815
Conversation
DanielMcInnes
force-pushed
the
1232-redesigned-top-level-structure-for-the-settings
branch
3 times, most recently
from
c4f60d8 to
7d35c97
Compare
|
NB: The design has 'Console Password' and 'Enable console on VRM', these are no longer required https://victrondevelopment.slack.com/archives/C020QG3JVEW/p1734675975736589
|
DanielMcInnes
force-pushed
the
1232-redesigned-top-level-structure-for-the-settings
branch
from
405018f to
b71436d
Compare
There was a problem hiding this comment.
Great work! The settings are looking so much better. Nice work on refactoring into components as well.
For each commit message, could you:
- Add a reference to the task number in each commit message
- Make the commit message more descriptive - e.g. "Move Date & Time to General settings" instead of just "Date & Time", or "Add PageSettingsDisplayAndAppearance" instead of just "PageSettingsDisplayAndAppearance".
Also it would be nice to squash some of the smaller related commits, e.g. the four separate commits that add the extra bottom items to the Device List page.
| id: root | ||
|
|
||
| GradientListView { | ||
| id: settingsListView |
There was a problem hiding this comment.
The existing PageSettingsGeneral contains the magic up/down key combinations and list drag feature to change access level. Now that the access level is shown on this page instead, those features should be moved into this page instead.
| import QtQuick | ||
| import Victron.VenusOS | ||
|
|
||
| Item { |
There was a problem hiding this comment.
Is the separate Item necessary or can the Label be the root item, if the topPadding and bottomPadding are set? And looking at the design, there is slightly more space above the text than below it:
There was a problem hiding this comment.
updated as suggested
pages/settings/PageSettingsVrm.qml
Outdated
|
|
||
|
|
||
| GradientListView { | ||
| id: settingsListView |
|
|
||
| ListMqttAccessSwitch { | ||
| id: mqtt | ||
| } | ||
|
|
||
| Column { |
There was a problem hiding this comment.
Looks like PageSettingsServices can be removed now, though "CAN-bus over TCP/IP (Debug)" also needs to be moved to the connectivity page alongside the other VE.Can list items.
There was a problem hiding this comment.
do you have a way of detecting unused qml files in a project?
| ListNavigation { | ||
| //% "Modbus TCP Server" | ||
| text: qsTrId("pagesettingsintegrations_modbus_tcp_server") | ||
| onClicked: Global.pageManager.pushPage("/pages/settings/PageSettingsModbusTcp.qml", {"title": text}) // TODO - is this correct? |
There was a problem hiding this comment.
The list item seems correctly placed according to the new design structure - is there something else that needs to be resolved for this TODO?
There was a problem hiding this comment.
This was a leftover comment, removed now.
| } | ||
| } | ||
|
|
||
| ListMqttAccessSwitch { } // TODO - is this correct? |
There was a problem hiding this comment.
Regarding the TODO, this seems correctly placed according to the new design structure? Though in the design it says "MQTT server" instead of "MQTT access".
There was a problem hiding this comment.
The naming was switched with the security update made a while ago. Probably it was not updated in the Figma. MQTT Access is the correct one.
| } | ||
| } | ||
|
|
||
| ListNavigation { |
There was a problem hiding this comment.
I wonder if "Tailscale" should go below "Mobile network", seems more related to that group than to GPS and CAN devices
There was a problem hiding this comment.
I would put it under Access & Security, since it‘s for remote access.
There was a problem hiding this comment.
Moved "Tailscale" below "Mobile network"
DanielMcInnes
force-pushed
the
1232-redesigned-top-level-structure-for-the-settings
branch
2 times, most recently
from
6fa6e4a to
35311d5
Compare
| ListItem { | ||
| id: root | ||
|
|
||
| property alias text: primary.text |
There was a problem hiding this comment.
This looks pretty much the same as ListSwitch but with a caption? Can the caption just be added to ListSwitch using bottomContent?
There was a problem hiding this comment.
I couldn't do it using bottomContent, it didn't look right. Done a different way, removed SettingsListSwitch.
DanielMcInnes
force-pushed
the
1232-redesigned-top-level-structure-for-the-settings
branch
from
35311d5 to
f133bb1
Compare
Done |
| keyEvents.downCount = 0 | ||
| } | ||
| } | ||
| boundsBehavior: Flickable.DragOverBounds |
There was a problem hiding this comment.
This boundsBehavior: Flickable.DragOverBounds needs to move to the listview in PageSettingsAccessAndSecurity, otherwise that list view cannot be dragged to change to superuser.
| } | ||
| //% "Incorrect password" | ||
| return Utils.validationResult(VenusOS.InputValidation_Result_Error, qsTrId("settings_access_incorrect_password")) | ||
| } | ||
| } |
There was a problem hiding this comment.
"Network security profile" doesn't make sense here anymore, should it be moved to the "Access and security" page, e.g. as the second item?
| allowed: tailscale.isValid | ||
|
|
||
| VeQuickItem { | ||
| id: tailscale |
There was a problem hiding this comment.
I'm getting this error when trying to open the page:
qml: Aborted attempt to push page with errors: /pages/settings/PageSettingsConnectivity.qml: qrc:/qt/qml/Victron/VenusOS/pages/settings/PageSettingsConnectivity.qml:119 id is not unique
There was a problem hiding this comment.
whoops, fixed
pages/settings/debug/PageDebug.qml
Outdated
| text: "System data" | ||
| onClicked: Global.pageManager.pushPage("/pages/settings/debug/PageSystemData.qml", { title: text }) | ||
| } | ||
|
|
||
| ListNavigation { | ||
| text: "Test" |
There was a problem hiding this comment.
Could you rename "Test" to "UI library" while you're at it?
DanielMcInnes
force-pushed
the
1232-redesigned-top-level-structure-for-the-settings
branch
from
f133bb1 to
a4ea0b4
Compare
No description provided.