Skip to content
This repository was archived by the owner on Oct 11, 2022. It is now read-only.

Conversation

@brianlovin
Copy link
Contributor

@brianlovin brianlovin commented May 30, 2018

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • hyperion (frontend)

  • Improves the splash loading view

  • Makes navbar and titlebar flush with the window to feel more native ✨

  • closes #3187

  • closes #3179

  • closes #3180

@spectrum-bot
Copy link

spectrum-bot bot commented May 30, 2018

Warnings
⚠️

These modified files do not have Flow enabled:

  • src/views/navbar/style.js

Generated by 🚫 dangerJS

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

This will need a new desktop app release so I guess this is a great time to test the auto updating!

width: 100%;
height: 100%;
background: url('../../public/img/media.png') center center no-repeat;
background: url('../../public/img/homescreen-icon-144x144.png') center center no-repeat;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we showing the icon there instead of the full "Spectrum"? Not sure how that's better?

Copy link
Contributor

@mxstbr mxstbr May 30, 2018

Choose a reason for hiding this comment

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

This looks very weird?

screen shot 2018-05-30 at 10 10 32

Not a fan at all, I much preferred the previous version with the Spectrum. Going to revert this.

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

I assume those buttons aren't where they should be?

screen shot 2018-05-30 at 10 11 10

width: 100%;
height: 100%;
background: url('../../public/img/homescreen-icon-144x144.png') center center no-repeat;
background: url('../../public/img/media.png') center center no-repeat;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did you revert this?

Copy link
Contributor

@mxstbr mxstbr May 30, 2018

Choose a reason for hiding this comment

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

Look above in the collapsed comment, I really really do not like that. It looks bad in comparison to showing the media.png.

Copy link
Contributor

Choose a reason for hiding this comment

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

Look at this comparison:

screen shot 2018-05-30 at 10 10 32

screen shot 2018-05-30 at 16 18 20

The second one looks so much more like a start screen to me?

Copy link
Contributor

@mxstbr mxstbr May 30, 2018

Choose a reason for hiding this comment

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

On a scale from 1 to 10 I'm maybe a 4 on how much I care about this, if you're 10/10 convinced we should go with the first one then I'm fine with it, but at least use the actual app icon and not the homescreen icon for the PWA which is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I disagree here, too - I think the media version is much clunkier and feels less like someone is opening a native app. Having the PWA icon as a squircle with the shadows.

I'd rather have nothing at all and just let people wait a second or two for startup.

I see other native mac apps do this too: just show nothing during boot.

@brianlovin
Copy link
Contributor Author

And those buttons are where they should be - this solves the problem of removing the native titlebar on macOS. What do you think?

@mxstbr
Copy link
Contributor

mxstbr commented May 30, 2018

And those buttons are where they should be - this solves the problem of removing the native titlebar on macOS. What do you think?

I don't think it looks better, honestly I much prefer the grey bar. I think them being off-center looks pretty crappy.

@brianlovin
Copy link
Contributor Author

I don't think it looks better, honestly I much prefer the grey bar. I think them being off-center looks pretty crappy.

I disagree - the gray bar makes us the app look like a lazily-wrapped website (which it is under the hood). Hiding that bar and merging our UI with native UI feels much cleaner and more app-like. I can play with the button centering, it'll just mean tightening up our app UI, although to be honest the way it is in this branch doesn't look bad to me.

@mxstbr
Copy link
Contributor

mxstbr commented May 30, 2018

I think it looks really really bad. 😕

Maybe we can just add a 38px black bar atop the navbar where that the buttons then sit in the center of?

@brianlovin
Copy link
Contributor Author

Maybe we can just add a 38px black bar atop the navbar where that the buttons then sit in the center of?

Sure, I'll play with it

@brianlovin
Copy link
Contributor Author

Here's what this will look like with hiddenInset title bar style - just adds a bit of extra padding to the traffic lights
screenshot 2018-05-30 08 01 23

Versus how things look today:
screenshot 2018-05-30 08 02 15

IMO I think the flush version looks and feels much better, more native.

@brianlovin
Copy link
Contributor Author

screenshot 2018-05-30 08 13 27

Here it is tidied up and centered.

@mxstbr
Copy link
Contributor

mxstbr commented May 30, 2018

I much much much prefer the centered version, that looks like an actual native app!

@brianlovin
Copy link
Contributor Author

Sounds good - those changes are pushed!

Anything else here @mxstbr ? Thanks for the feedback so far

@brianlovin brianlovin merged commit 6cf4e99 into alpha May 30, 2018
@brianlovin brianlovin deleted the desktop-polish branch May 30, 2018 17:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Desktop] Unable to open a new window [Desktop] Command + [ or ] should navigate forward/backwards [Desktop] Cmd + H should hide the app

3 participants