Create vertical layout option #11

mhkeller · 2023-04-08T05:19:18Z

#9 Creates an option in settings to have a vertical layout, similar to Dark Sky

Here's a first go at it. Needs more mobile testing and check if the math is right aligning the times alongside the vertical bar and that the temperatures are scaled correctly.

Let me know what you think.

mhkeller · 2023-04-08T16:18:57Z

I'm playing around with some details for aligning the weather timestamps to the vertical bar. In Dark Sky, it appears that the timestamp falls near the middle of that bar. So hours 8am and 10am are labeled and rain starts at 9am the blue bar will start halfway between 8am and 10am. Setting a height of 100 / 12 sets the label a little differently. I'm trying to get it to align center without throwing off the first label... Work in progress...

mhkeller · 2023-04-08T20:35:58Z

@vsergeev I think it's ready for you to take a look. Let me know what you think:

vsergeev · 2023-04-14T08:37:03Z

@mhkeller thanks, this looks like a good start. I have a couple of feedback items:

I think it would look better at 80% width and centered
The temperature pills aren't centered with the line for me
I'd like to avoid any fixed pixel heights on containers (e.g. height: 525px for the container in HourlyDetails)
Commit 9679c95 appears to remove text colors and drop shadows, which breaks the formatting on the horizontal view. (Why were these deleted?)
No need to commit the production bundle -- I can do that on an actual release with a version number change

The commits can also be squashed down into two commits, one that adds the layout setting (first commit), and one that adds the vertical layout.

mhkeller · 2023-04-14T13:32:44Z

Woops on 9679c95. I'll revert that and work on the other items. For the height, what would you prefer, something like 80vh / 90vh / 100vh?

I noticed that there is no sunrise / sunset information for the current day anymore because that is only in the expandable component. I'll add those elements to the text information at the top.

vsergeev · 2023-04-14T15:52:23Z

Yeah, I think a relative view height would be good in place of the fixed height.

mhkeller · 2023-04-15T04:31:11Z

Some updates:

The temperature pills aren't centered with the line for me

This should be fixed.

I'd like to avoid any fixed pixel heights on containers (e.g. height: 525px for the container in HourlyDetails)

Fixed. Removing the height seemed to work fine.

Commit 9679c95 appears to remove text colors and drop shadows, which breaks the formatting on the horizontal view. (Why were these deleted?)

Fixed

No need to commit the production bundle -- I can do that on an actual release with a version number change

Fixed

The commits can also be squashed down into two commits, one that adds the layout setting (first commit), and one that adds the vertical layout.

Fixed

I think it would look better at 80% width and centered

This is the only thing that didn't work out well.The width becomes too narrow for there to be much variation on the temperature scale. Here's a screenshot.

Separately:

I added the sunrise and sunset times back and I standardized their look between the two components.

mhkeller · 2023-04-15T20:55:02Z

I increased the padding in some places for better tap accessibility and lightened up the temperature range so it's more easily scannable to the eye.

mhkeller · 2023-04-15T20:57:46Z

One other thing that's missing from today's view is when it's raining, it should display the Precipitation and Amount fields.

vsergeev

This PR introduces unrelated changes and breaks the default color scheme and formatting. I think it's headed in the right direction overall, but I have to emphasize that the PR should have absolutely no impact on the rest of the app or to the existing horizontal layout. It also shouldn't try to incorporate new features that are outside the scope of the PR ("Create vertical layout option").

High level feedback:

Adding precipitation and sunrise/sunset information to the top details should be a separate discussion and possibly PR. In it's current form, I'm not willing to add those fields because they're poorly formatted on desktop with two line rows that impair the readability of the other fields, and it's an open question whether these fields here add to the app or compromise its simplicity.
For the width, we can use a media query to be 100% in mobile and 75% on desktop, so it's easy to read the vertical layout on both (without excessive left-right eye scanning on desktop).
The temperature pills are vertically aligned with the lines, but I think it would look better if the hour and description on the left were vertically aligned with the lines too.
Please compare the effect of your branch with the current deployment in both mobile and desktop. There are some regressions -- I've attached two screenshots as examples:

Original:

This Branch:

vsergeev · 2023-04-19T04:00:02Z

package.json

@@ -4,7 +4,7 @@
  "version": "1.0.0",
  "type": "module",
  "scripts": {
-    "dev": "vite",
+    "dev": "vite --host",


This doesn't belong in this commit or PR.

This is just to help test and can be removed before the final merge

vsergeev · 2023-04-19T04:00:59Z

src/components/DailyPrecipitationDetails.svelte

+
+{#if precipitation.precipitation_probability !== undefined && precipitation.precipitation_amount !== undefined}
+	<div>
+		<span class="text-sm md:text-base font-semibold">Precipitation:</span> <span class="text-sm md:text-base">{precipitation.precipitation_probability}%</span>


These line indents need to be normalized. You can run npm run format to format automatically.

vsergeev · 2023-04-19T04:02:16Z

src/components/scalars/TemperatureRange.svelte

@@ -19,7 +19,7 @@
  <div class="text-right min-w-fit" style="min-width: {margin_left === 0 ? 'min-content' : `${margin_left}%`};">
    <Temperature value={low} />
  </div>
-  <div class="bg-gray-800 dark:bg-gray-400 my-0.5 mx-1.5 rounded-xl" style="width: {100 - margin_right - margin_left}%;" />
+  <div class="bg-gray-400 dark:bg-gray-400 my-0.5 mx-1.5 rounded-xl" style="width: {100 - margin_right - margin_left}%;" />


This change affects the default color scheme...

I'll put this in a separate PR

vsergeev · 2023-04-19T04:10:41Z

src/providers/Provider.ts

@@ -74,6 +74,15 @@ export interface ProviderFactory {
  fromParams(params: object, location?: Location): Provider | null;
 }

+export interface SunTimes {


Types here should be limited to the boundary between the backend provider and the frontend, not auxiliary types for components. I think those components are simple enough that they aren't really necessary, anyway.

mhkeller · 2023-04-19T05:06:00Z

Sure thing. I incorporated some accessibility and duplicate content fixes that I encountered but can create new issues for those. I can explain the reasoning for them in those issues and would be in favor of including them since there is some duplicate information that would adversely affect the UX for the vertical layout.

mhkeller · 2023-04-19T05:22:04Z

The temperature pills are vertically aligned with the lines, but I think it would look better if the hour and description on the left were vertically aligned with the lines too.

Can you explain what you mean by this? Do you mean the horizontal line looks like it's not at the centerline of the text? Maybe you can send a screenshot of what you're seeing so I can make sure the fix is getting at the right thing.

vsergeev · 2023-04-19T05:25:16Z

The temperature pills are vertically aligned with the lines, but I think it would look better if the hour and description on the left were vertically aligned with the lines too.

Can you explain what you mean by this? Do you mean the horizontal line looks like it's not at the centerline of the text? Maybe you can send a screenshot of what you're seeing so I can make sure the fix is getting at the right thing.

I think so. Sure, here is a screenshot:

mhkeller · 2023-04-19T05:37:00Z

Got it. Yea that looks different from mine. What browser and OS are you using so I can try to replicate?

vsergeev · 2023-04-19T05:40:44Z

Got it. Yea that looks different from mine. What browser and OS are you using so I can try to replicate?

I'm using Firefox 111 on Linux. I see it in Google Chrome 111 too.

vsergeev · 2023-04-27T04:43:15Z

I've added a View tab to the Settings on master, which will be a better place for a layout option like this.

msft-jeelpatel · 2024-01-14T21:15:01Z

The temperature pills are vertically aligned with the lines, but I think it would look better if the hour and description on the left were vertically aligned with the lines too.

Can you explain what you mean by this? Do you mean the horizontal line looks like it's not at the centerline of the text? Maybe you can send a screenshot of what you're seeing so I can make sure the fix is getting at the right thing.

I think so. Sure, here is a screenshot:

Hi @mhkeller. Were you able to address this?

mhkeller · 2024-01-14T22:55:31Z

@msft-jeelpatel, I didn't have the bandwidth to test this out since it was fine on my machine and I don't a Linux to test on. I won't have the time to finish this PR but if someone wants to pick it up, feel free to! I'll close this and if someone wants to resurrect it, then by all means they should

mhkeller · 2024-01-23T16:19:49Z

Reopening if other ppl want to work on it

patel-jeel92 · 2024-01-23T21:58:53Z

Reopening if other ppl want to work on it

I wish I knew how to fix some of the layout issues :/.

For example, the "Now" starts at the very top of the weather bar

Compared to DarkSky it looked like there was equal padding at the top and bottom. Currently, since the "Now" starts at the very top margin of the weather bar, there is quite a lot of unused space at the bottom

Other than that, I can try taking this forward

vsergeev · 2024-01-25T09:16:45Z

@patel-jeel92 I can help with some of the formatting issues to get this PR to the finish line.

patel-jeel92 · 2024-01-25T15:48:03Z

Hi @vsergeev I have started a new PR #21 to add the vertical layout. The reason i chose to add a new PR is because i have forked the repo and created a new branch off of it. Plus this had a bunch on conflicts and i found it easier to start over.

Huge thanks to @mhkeller for starting the initial work!

add layout setting

0d1369f

mhkeller force-pushed the vertical-layout branch 2 times, most recently from b715171 to 899cf36 Compare April 15, 2023 04:24

Create vertical layout

e16831a

mhkeller force-pushed the vertical-layout branch from 899cf36 to e16831a Compare April 15, 2023 20:53

Add precipitation to daily values

35a9116

vsergeev requested changes Apr 19, 2023

View reviewed changes

vsergeev force-pushed the master branch 2 times, most recently from 6ba04fc to 91e8eef Compare June 28, 2023 06:57

mhkeller closed this Jan 14, 2024

mhkeller reopened this Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create vertical layout option #11

Create vertical layout option #11

mhkeller commented Apr 8, 2023

mhkeller commented Apr 8, 2023

mhkeller commented Apr 8, 2023

vsergeev commented Apr 14, 2023

mhkeller commented Apr 14, 2023

vsergeev commented Apr 14, 2023

mhkeller commented Apr 15, 2023 •

edited

Loading

mhkeller commented Apr 15, 2023

mhkeller commented Apr 15, 2023

vsergeev left a comment

vsergeev Apr 19, 2023

mhkeller Apr 19, 2023

vsergeev Apr 19, 2023

vsergeev Apr 19, 2023

mhkeller Apr 19, 2023

vsergeev Apr 19, 2023

mhkeller commented Apr 19, 2023

mhkeller commented Apr 19, 2023

vsergeev commented Apr 19, 2023

mhkeller commented Apr 19, 2023

vsergeev commented Apr 19, 2023

vsergeev commented Apr 27, 2023

msft-jeelpatel commented Jan 14, 2024 •

edited

Loading

mhkeller commented Jan 14, 2024

mhkeller commented Jan 23, 2024

patel-jeel92 commented Jan 23, 2024

vsergeev commented Jan 25, 2024

patel-jeel92 commented Jan 25, 2024

Create vertical layout option #11

Are you sure you want to change the base?

Create vertical layout option #11

Conversation

mhkeller commented Apr 8, 2023

mhkeller commented Apr 8, 2023

mhkeller commented Apr 8, 2023

vsergeev commented Apr 14, 2023

mhkeller commented Apr 14, 2023

vsergeev commented Apr 14, 2023

mhkeller commented Apr 15, 2023 • edited Loading

mhkeller commented Apr 15, 2023

mhkeller commented Apr 15, 2023

vsergeev left a comment

Choose a reason for hiding this comment

vsergeev Apr 19, 2023

Choose a reason for hiding this comment

mhkeller Apr 19, 2023

Choose a reason for hiding this comment

vsergeev Apr 19, 2023

Choose a reason for hiding this comment

vsergeev Apr 19, 2023

Choose a reason for hiding this comment

mhkeller Apr 19, 2023

Choose a reason for hiding this comment

vsergeev Apr 19, 2023

Choose a reason for hiding this comment

mhkeller commented Apr 19, 2023

mhkeller commented Apr 19, 2023

vsergeev commented Apr 19, 2023

mhkeller commented Apr 19, 2023

vsergeev commented Apr 19, 2023

vsergeev commented Apr 27, 2023

msft-jeelpatel commented Jan 14, 2024 • edited Loading

mhkeller commented Jan 14, 2024

mhkeller commented Jan 23, 2024

patel-jeel92 commented Jan 23, 2024

vsergeev commented Jan 25, 2024

patel-jeel92 commented Jan 25, 2024

mhkeller commented Apr 15, 2023 •

edited

Loading

msft-jeelpatel commented Jan 14, 2024 •

edited

Loading