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.

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 #7744, make logout screen nicer #7747

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

pascalwengerter
Copy link
Contributor

Description

See changelog item

Related Issue

Screenshots (if appropriate):

@tbsbdr opted for a primary button (since it's the only one on the screen, and using a different one would add extra effort to make it compatible with bright/dark mode).

Screenshot 2022-10-06 at 22-38-24 Access denied - ownCloud

Types of changes

  • New feature (non-breaking change which adds functionality)

@ownclouders
Copy link
Contributor

Results for oC10Basic https://drone.owncloud.com/owncloud/web/28912/13/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUILogin-oauthLogin_feature-L41.png

webUILogin-oauthLogin_feature-L41.png

webUILogin-oauthLogin_feature-L49.png

webUILogin-oauthLogin_feature-L49.png

💥 The oC10Basic tests pipeline failed. The build has been cancelled.

@tbsbdr
Copy link
Contributor

tbsbdr commented Oct 7, 2022

Thanks, Pascal!

Polishing proposal:
I'd propose to have the same width for the button and the card.
Width should be the same like on the login-screen (300 px / oc-width-medium )

Same width for button and card:

Screenshot 000726@2x

login-screen width:

Screenshot 000727@2x

@JammingBen
Copy link
Collaborator

@pascalwengerter Thanks for working on this!

This is actually pretty urgent, we need this merged today. Do you think you can apply Tobis proposal and fix the tests, or should I finish it?

@pascalwengerter
Copy link
Contributor Author

@pascalwengerter Thanks for working on this!

This is actually pretty urgent, we need this merged today. Do you think you can apply Tobis proposal and fix the tests, or should I finish it?

Ah damn, let me check if I can quickly fit it. You're free to work on it from 13:30 if I haven't pushed anything by then;)

@pascalwengerter
Copy link
Contributor Author

@JammingBen please review, should be fine now

Copy link
Collaborator

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Nice 😍 oc-width-medium would be nice, the rest is awesome!

<oc-button
id="exitAnchor"
type="router-link"
class="oc-mt-m oc-width-large"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class="oc-mt-m oc-width-large"
class="oc-mt-m oc-width-medium"

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 tried medium and it was definitely smaller than the parent (see previous comment above)


We have updated the message and layout of the logout screen to make it a more pleasant user experience.

https://github.com/owncloud/web/pull/7748
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
https://github.com/owncloud/web/pull/7748
https://github.com/owncloud/web/pull/7747

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmgrpfff @kulmann can I get a force-merge if the rest of CI is happy and I fix this?

Copy link
Member

Choose a reason for hiding this comment

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

Depends on what else has been changed 😅 noted that the CI was green just now.

<div class="oc-login-card-wrapper oc-height-viewport oc-flex oc-flex-center oc-flex-middle">
<div class="oc-login-card">
<img class="oc-login-logo" :src="logoImg" alt="" :aria-hidden="true" />
<div class="oc-login-card-body oc-width-large">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JammingBen this is already width-large (and was before I changed things) so adding medium to the parent wouldn't make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry, then using oc-width-medium here and on the button would be the way to go:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright let's go then, just a sec

@pascalwengerter
Copy link
Contributor Author

@pascalwengerter Thanks for working on this!
This is actually pretty urgent, we need this merged today. Do you think you can apply Tobis proposal and fix the tests, or should I finish it?

Ah damn, let me check if I can quickly fit it. You're free to work on it from 13:30 if I haven't pushed anything by then;)

Also please note that having this merged/published would include one transifex-roundtrip for fresh translations 😇

@sonarcloud
Copy link

sonarcloud bot commented Oct 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Collaborator

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Nice 🚀 Sorry for the rush!

license/cla is having issues again though. Might be that you need to push again :/ Edit: Nvm, works now!

@JammingBen JammingBen merged commit 6bb4f73 into owncloud:master Oct 7, 2022
@tbsbdr
Copy link
Contributor

tbsbdr commented Oct 10, 2022

added translations in transifex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wording: Make "Logout-Message" friendlier
5 participants