Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Fix For BlockClock Min Size #385

Merged
merged 1 commit into from
Jan 1, 2024
Merged

Fix For BlockClock Min Size #385

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/qml/components/BlockClock.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ Item {
id: dial
anchors.horizontalCenter: root.horizontalCenter
scale: Theme.blockclocksize
width: Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale))
width: {Math.max(Math.min(200, Math.min(root.parentWidth - 30, root.parentHeight - 30)),
Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale)))}
Comment on lines +45 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would extract the formula to a function where I'd structure the logic as @GBKS suggested above.

How about this...** 
--- a/src/qml/components/BlockClock.qml
+++ b/src/qml/components/BlockClock.qml
@@ -44,7 +44,7 @@ Item {
         id: dial
         anchors.horizontalCenter: root.horizontalCenter
         scale: Theme.blockclocksize
-        width: Math.min((root.parentWidth * dial.scale), (root.parentHeight * dial.scale))
+        width: calculateConstrainedWidth()
         height: dial.width
         penWidth: dial.width / 50
         timeRatioList: chainModel.timeRatioList
@@ -239,6 +239,24 @@ Item {
         }
     ]

+    function calculateConstrainedWidth() {
+        // Margin size used to create a buffer around the constrained area.
+        var marginSize = 30;
+
+        // The maximum size.
+        // This is our hardest constraint - the physical screen size.
+        var screenConstraintSize = Math.min(root.parentWidth, root.parentHeight) - marginSize;
+
+        // The minimum size (for legibility).
+        var minimumSize = 200;
+
+        // The dynamic target size based on our display mode scale.
+        // Basically just scaling up for larger non-mobile screens.
+        var scaleTargetSize = Math.min(root.parentWidth, root.parentHeight) * dial.scale;
+
+        // Mashing them all up
+        return Math.min(screenConstraintSize, Math.max(minimumSize, scaleTargetSize))
+    }

     function formatProgressPercentage(progress) {
         if (progress >= 1) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for testing on a wearable device, maybe let’s open a new PR if this feels like a priority, what do you think @GBKS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Including the explicit logic makes it easier to read, but since I have no clue about the established code standards in this project, my opinion here should not weigh much.

We should address the separate layout at tiny scales in a different PR. I'll try to mock up all the variations, and also include them in the design docs page, probably early next week.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mocked things up and added them to the design docs, see here. Feedback appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also had a question about how this works on Android Wear. Is the full app running on the watch? Or does it run in some sort of stripped down way? I'd be surprised of watches are powerful enough. I also assume we can detect if the app runs on a watch and potentially adjust the layout accordingly (specifically for the rounded frame)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the full app running on the watch? Or does it run in some sort of stripped down way?

I haven't passed the "Connecting..." phase, even the watch is connected to the internet and everything, I'd need to debug and check deeper, but I doubt it, I was just going a bit further with the testing of this PR and since I was going thru the Android guide it seems I got a bit overexcited 😆.

I'd be surprised of watches are powerful enough.

Well, this is an old one, 2017-ish, nowadays resources on smartwatches are still limited but they are getting there.

I also assume we can detect if the app runs on a watch and potentially adjust the layout accordingly (specifically for the rounded frame)?

I'd think so, I'll check when I do something related with it.

height: dial.width
penWidth: dial.width / 50
timeRatioList: chainModel.timeRatioList
Expand Down