Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Indicate presence of a Settings page #411

Merged
merged 2 commits into from
Feb 6, 2023

Conversation

nephros
Copy link
Contributor

@nephros nephros commented Feb 3, 2023

some patches come with a settings page.

users often are not aware, so lets add a hint

nephros added 2 commits February 3, 2023 22:55
... may or may not make it more readable ;)
before it was *roughly* functionally grouped.
@nephros
Copy link
Contributor Author

nephros commented Feb 3, 2023

the first commit may seem confusing but it just makes the order of elements more readable (to me).
the real change is in the second commit.

@Olf0
Copy link
Contributor

Olf0 commented Feb 4, 2023

Ugh, lots of changes in QML code even if they are partially repetitive. Hence I rather ask @b100dian to review this.

P.S.: BTW @nephros, I posed a hidden, open question WRT a potentially stale branch for the unmerged and closed PR #410, which was superseded by PR #409.

@nephros
Copy link
Contributor Author

nephros commented Feb 5, 2023

Screenshot of the change:

Screenshot_20230204_001_1_1

The text will be "tap to configure" while the patch is applied/activated, and "tap to show configuration" when it is not.

That's all, the rest is just some cleanups.

@Olf0
Copy link
Contributor

Olf0 commented Feb 5, 2023

Hey @nephros, I never doubt that you did that (or anything else) with due diligence and in fact your screenshot is looking really good. I now took a closer look at the real changes in commit f582439 (i.e., sans the shuffling in commit 67207b1) and they appear to be fine to me. Still I only can check for obvious typos and syntax / formatting errors, but little beyond that: Subtle logical flaws will likely escape my perception when reading QML code.

So I can approve this if you think it is pressing, but still would appreciate if @b100dian does that.

Label {
text: name
color: patchObject.details.isCompatible ? background.down ? Theme.highlightColor : Theme.primaryColor
: background.down ? Qt.tint(Theme.highlightColor, "red") : Qt.tint(Theme.primaryColor, "red")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unrelated) wondering if we should use some alpha on this color tinting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh you're right, this looks like it doesn't actually "tint" at all, it just results in red.

Also we might just change to Theme.errorColor, which has always been red but might change at some point.

@nephros nephros merged commit 3fb7a2d into sailfishos-patches:master Feb 6, 2023
@nephros nephros deleted the show-settings branch February 6, 2023 07:59
nephros pushed a commit to nephros/patchmanager that referenced this pull request Feb 6, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants