Conversation
spiros132
left a comment
spiros132
left a comment
There was a problem hiding this comment.
Some minor feedback about the PR :)
There was a problem hiding this comment.
You don't need to use qmldir, instead just import the relative folder and it will import the components in the folder as well.
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "id": "noctalia-vantage", | |||
| "name": "Noctalia Vantage", | |||
There was a problem hiding this comment.
Since we know that this is a plugin for the noctalia-shell, it might be better for the name to be something that doesn't contain noctalia. I'm not sure if this is a good idea but for example "Vantage Driver" or maybe "Vantage Controller".
There was a problem hiding this comment.
Makes sense, I initially went with "Noctalia Vantage" following the name pattern used in other environments:
- PlasmaVantage for KDE Plasma.
- CinnamonVantage for Cinnamon.
But I agree it's probably redundant here.
What do you think about "VantageCtl" as an alternative?
| "minNoctaliaVersion": "3.6.0", | ||
| "author": "Sourajyoti Basak", | ||
| "license": "GPL-3.0-or-later", | ||
| "repository": "https://github.com/wizard-28/noctalia-plugins", |
There was a problem hiding this comment.
This should be the noctalia-plugins repository
There was a problem hiding this comment.
Same here, you don't need the qmldir, just import the relative folder. For example:
import "./ui"
// Or
import "./ui/settings"| property bool writeable: false | ||
| property var value: null | ||
| property var validValues: null | ||
| property var transformWrite: v => typeof v === "boolean" ? (v ? 1 : 0) : v |
There was a problem hiding this comment.
This is a very confusing lambda function, can it be made to a regular function instead?
| } | ||
|
|
||
| return v === 1; | ||
| } |
There was a problem hiding this comment.
I would recommend all of these to be regular functions, there's no need for them to be properties
| root.reload(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This process already contains an id, I would suggest turning the root component to an Item instead, that way you don't need to define properties for each component.
| Logger.d("NoctaliaVantage", `${root.label} unchanged:`, parsed); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Here as well, prefer for the root component to be an item so that you can have this component inside it without assigning it to a property
2 participants
Repo: https://github.com/wizard-28/NoctaliaVantage
Allows users to change control their Lenovo ideapad/legion laptop settings through a panel widget.
Continuation of #493, which I had to close due to some issues