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

Fixes minor styling issues of admin and admin v3 in the dashboard #11977

Merged
merged 12 commits into from
Jan 3, 2024

Conversation

drummer83
Copy link
Contributor

@drummer83 drummer83 commented Dec 24, 2023

What? Why?

Before:
image

image

After:
image

image

What should we test?

  • Activate admin style v3
  • Make sure you have multiple enterprises but no order cycle open and no products.
  • Visit the overview page /admin.
  • Check that there is no thin blue border around the 'Manage order cycles field'.
  • Check that the products section looks identical.
  • Create a product.
  • Open an order cycle.
  • Check that the text 'You have x active ...' has the same color for products and order cycles.
  • Check that the other pages remain unchanged and everything is working.

Additional changes which could be tested:

  • With admin style v3 deactivated:

    • With products and order cycles active
      • Check that clickable areas turn into green when hovering
      • Check that clickable areas have no line when focusing them via TAB key
    • With NO products and NO order cycles active
      • Same as above.
    • With less than 6 enterprises available
      • Check that the enterprise list does not have a scroll bar (allowing to scroll only few px)
    • With 7 or more enterprises available
      • Check that the enterprise list has no border when focusing via TAB key
  • With admin style v3 activated:

    • With products and order cycles active
      • Check that clickable areas turn into darker blue when hovering
      • Check that clickable areas have a thin dark blue line when focusing them via TAB key
    • With NO products and NO order cycles active
      • Same as above.
    • With less than 6 enterprises available
      • Check that the enterprise list does not have a scroll bar (allowing to scroll only few px)
    • With 7 or more enterprises available
      • Check that the enterprise list has a thin dotted border when focusing via TAB key (Firefox only?)

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@drummer83 drummer83 changed the title Fixes minor styling issues of admin and admin v3 in the multi-enterprise dashboard Fixes minor styling issues of admin and admin v3 in the dashboard Dec 24, 2023
@drummer83 drummer83 marked this pull request as ready for review December 24, 2023 18:45
@drummer83
Copy link
Contributor Author

drummer83 commented Dec 24, 2023

A question for @mariocarabotta:
Should we make the 'focused' pseudo-class identical with 'hover'? The focus is very difficult to see currently when using the TAB key (just a 1px line).

A question for @dacook and @mariocarabotta:
Should we get rid of the spree color definitions in admin style v3? We still have these in use in the dashboard:

$spree-blue = $color-btn-bg
$spree-green = ?
$spree-light-blue = ?

@mariocarabotta
Copy link
Collaborator

A questions for @mariocarabotta: Should we make the 'focused' pseudo-class identical with 'hover'? The focus is very difficult to see currently when using the TAB key (just a 1px line).

I agree it's actually quite difficult to see. @dacook had re-enabled the outline default a few weeks ago.
I am not 100% sure what this means "Should we make the 'focused' pseudo-class identical with 'hover'?" - apologies if i'm misunderstanding it!
I had a look at a few websites and it seems like they increase the default line thickness (and style too..), but don't use the hover style when focused.
maybe we could increase the thickness of the dotted line? some examples from google and nngroup below.
Screenshot 2023-12-26 at 14 54 35
Screenshot 2023-12-26 at 14 53 27

A question for @dacook and @mariocarabotta: Should we get rid of the spree color definitions in admin style v3? We still have these in use in the dashboard:

$spree-blue = $color-btn-bg
$spree-green = ?
$spree-light-blue = ?

mmm this one I'm definitely not familiar with - will leave it to @dacook, sorry

@dacook
Copy link
Member

dacook commented Dec 28, 2023

Should we get rid of the spree color definitions in admin style v3?

They definitely need cleaning up, yes. But it will be hard to do that until we remove the old design, so for now we will keep them to enable easy switching between old/new design.

@dacook
Copy link
Member

dacook commented Dec 28, 2023

Should we make the 'focused' pseudo-class identical with 'hover'? The focus is very difficult to see currently when using the TAB key (just a 1px line).

There's three main states to consider here:

  • "Current" (the tab that's currently loaded)
  • Focus (keyboard navigation with tab)
  • Hover (mouse hover)

Here's a couple of examples (in that order)

Screen Shot 2023-12-28 at 3 31 39 pm
Screen Shot 2023-12-28 at 3 31 21 pm
Screen Shot 2023-12-28 at 3 34 09 pm

(there's a couple other states but these seem to be ignored in the above examples so I've also ignored them).

I like the blue outline for focus (this is browser default anyway) which is nice and clear, and I think the darkened background on hover is a good convention too.

I would consider these changes as lower priority (the current design doesn't inhibit use of the system) but would love to see it improved one day..

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great!

@RachL RachL added the pr-staged-fr staging.coopcircuits.fr label Jan 3, 2024
@RachL
Copy link
Contributor

RachL commented Jan 3, 2024

Looking good, thanks @drummer83 !!! Merging :)

@RachL RachL merged commit edfd427 into openfoodfoundation:master Jan 3, 2024
52 checks passed
@RachL RachL removed the pr-staged-fr staging.coopcircuits.fr label Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
5 participants