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

Jump to bottom

Fix apps list issues caused by redesign #33939

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Fix apps list issues caused by redesign #33939

merged 1 commit into from
Sep 8, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Sep 7, 2022 •
edited by szaimen
Loading

  • Make icons work on dark theme
  • Use new buttons (a18y ++)
  • Fix some eslint warnings

image

image

Fix #33926
Close #33861

@CarlSchwan CarlSchwan added this to the Nextcloud 25 milestone Sep 7, 2022
@CarlSchwan CarlSchwan requested a review from a team September 7, 2022 11:14
@CarlSchwan CarlSchwan self-assigned this Sep 7, 2022
@CarlSchwan CarlSchwan requested review from PVince81, artonge and skjnldsv and removed request for a team September 7, 2022 11:14
@skjnldsv
Copy link
Member

skjnldsv commented Sep 7, 2022

Not convinced by the choices of colours
I would do:

  • Enable = primary
  • Enable untested = secondary
  • Remove = error

@jancborchardt ?

skjnldsv
skjnldsv approved these changes Sep 7, 2022
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Other than the comment, good 👍

@CarlSchwan
Copy link
Member Author

Not convinced by the choices of colours I would do:

* Enable = primary

* Enable untested = secondary

* Remove = error

@jancborchardt ?

Fine by me, I'll change that 👍

@jancborchardt
Copy link
Member

@skjnldsv good point, I would even go with just text-only for "Remove". Otherwise we have red buttons all over.

@blizzz blizzz mentioned this pull request Sep 7, 2022
Merged
@CarlSchwan
Fix apps list issues caused by redesign
3fa59ba 
- Make icons work on dark theme
- Use new buttons
- Fix some eslint warnings

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Copy link
Member Author

New screenshot with the suggestions from Jan and 'John'

image

jancborchardt
jancborchardt approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks much much nicer and less scary, great work! :)

@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 8, 2022
szaimen
szaimen approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Nice!

@CarlSchwan
Copy link
Member Author

Ci failure unrelated (sharing acceptance tests)

@CarlSchwan CarlSchwan merged commit 6404e5b into master Sep 8, 2022
@CarlSchwan CarlSchwan deleted the fix/apps-list-redesign branch September 8, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nimishavijay nimishavijay nimishavijay approved these changes

@szaimen szaimen szaimen approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@artonge artonge Awaiting requested review from artonge

Assignees

@CarlSchwan CarlSchwan

Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility ui-refresh-feedback
Projects
None yet
Milestone
Nextcloud 25
5 participants
@CarlSchwan @skjnldsv @jancborchardt @szaimen @nimishavijay