-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Redesign guest pages for better accessibility #33265
Conversation
Looks great! :) 2 things:
|
Possible performance regression detected Show Output
|
Right this was a preexisting issue, fixed now ;)
I updated all the box to not be wide anymore by default unless specified otherwise like on the exception page. Screenshots of the new states are updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, looks so good now! :) Only 2 things:
- There is focus feedback on the username/password input field, but they don’t have a default border. Either using the component, or
border: 2px solid var(--color-border)
would look nice. - From the screenshots, all the scenarios except the internal server error look better with the narrow style – so I’d say let’s change the "Connect to your account" and "Account access" to narrow style. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the screenshots look great! :)
Not sure if this is relevant to this PR but we should change everything that was full opacity black #000000
to --color-main-text
, it's especially visible in the screenshot with the app icons.
</p> | ||
<p v-else-if="loadingAppsError" class="loading-error text-center"> | ||
{{ t('core', 'Could not fetch list of apps from the App Store.') }} | ||
</p> | ||
<p v-else-if="installingApps" class="text-center"> | ||
{{ t('core', 'Installing apps …') }} | ||
{{ t('core', 'Installing apps…') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's not shortcutting the word "apps" the space is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted, but this feels weird. Is this convention documented somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rakekniven @jancborchardt anyone has a link?
a9f5654
to
bf0cd74
Compare
9ec514d
to
53e4007
Compare
6ded9bc
to
464355a
Compare
39bd890
to
3d4d8e2
Compare
3d4d8e2
to
5d7d651
Compare
@jancborchardt looking at the screenshot shouldn't "Cancel" rather be "Skip" ? |
It should, good catch |
Thanks for caring and for taking care @CarlSchwan 💙 |
Fixed :) I would say this is good to go now. There might be some apps that need to be adapted after this change but I for the moment checked most of them and everything seems to still work fine |
forgot to push? Or Push failed? |
8e5cc5c
to
a94b222
Compare
forgot to push :( |
a94b222
to
84a2dbe
Compare
- Use white box and put content on it - Improve focus indicator Signed-off-by: Carl Schwan <[email protected]>
Signed-off-by: Carl Schwan <[email protected]>
b662450
to
9c94c91
Compare
Signed-off-by: Carl Schwan <[email protected]>
9c94c91
to
3fa6ee3
Compare
@CarlSchwan this looks so damn cool, and is much more accessible on top of it. Great work! ✨ |
nextcloud/server#33265 changed <input> to <button>. Signed-off-by: Daniel Kesselberg <[email protected]>
nextcloud/server#33265 changed <input> to <button>. Signed-off-by: Daniel Kesselberg <[email protected]>
Screenshots:
TODO: