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

Dark accordions #1025

Merged
merged 7 commits into from
Feb 14, 2022
Merged

Dark accordions #1025

merged 7 commits into from
Feb 14, 2022

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Jan 7, 2022

Description

Adding a .accordion-dark for a dark variant of accordions.

Motivation & Context

Need for orange footers

Types of changes

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

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Development

  • Should match specs (eg. either the Web UI Kit or any pattern from the WAI — or both…)
  • Docs added:
    • including the "Sass" part using scss-docs shortcode
    • in /about/overview/#custom-components if it is a new Orange custom component
    • in /getting-started/introduction/#components if it is a new Orange custom component that requires JavaScript (and Popper)
    • in /customize/overview#csps-and-embedded-svgs if it is a new Orange custom component that includes embedded SVGs in our CSS
    • in /forms/validation/?#supported-elements if it is a new Orange custom component that is a form control
    • in /forms/overview/ if it is a new Orange custom component that is a form control
  • Check (and fix) RTL version
  • Run linters
  • Run compilers
  • Tests added for JS-side
  • Run tests
  • Manually run BrowserStack test
  • Manually run Percy test
  • Cross-browser test:
    • Firefox ESR
    • Latest Edge, Chrome, Firefox, Safari
    • iOS Safari
    • Chrome & Firefox on Android
  • Clean up the branch using rebase -i
  • Commited with feat(…): … message
  • Mention it in Migration Guide (if back-from-v4): renamed variables, changes in markup requirement, etc.

Reviews

  • Code review
  • A11y review
  • Design review

Related issues

#917

Live preview

scss/_accordion.scss Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
@louismaximepiton louismaximepiton mentioned this pull request Jan 7, 2022
19 tasks
@julien-deramond
Copy link
Member

julien-deramond commented Jan 7, 2022

This time, pa11y-ci failed with a real error 😄

    Error: Elements must have sufficient color contrast (https://dequeuniversity.com/rules/axe/3.5/color-contrast?application=axeAPI)
    color-contrast
    <code>.accordion-body</code> (select with "#panelsDark-collapseOne > div > code")

You can try with a lighter gray. Another solution is to let the text black and highlight it with white.

Example of implementation:

diff --git a/scss/_accordion.scss b/scss/_accordion.scss
index 72636e5a6..5d8819124 100644
--- a/scss/_accordion.scss
+++ b/scss/_accordion.scss
@@ -149,5 +149,10 @@
       border-bottom: $accordion-border-width solid $accordion-dark-border-color;
     }
   }
+
+  var,
+  code {
+    color: $code-dark-color;
+  }
 }
 // End mod
diff --git a/scss/_variables.scss b/scss/_variables.scss
index ba383bed6..3f96338a5 100644
--- a/scss/_variables.scss
+++ b/scss/_variables.scss
@@ -1788,6 +1788,7 @@ $offcanvas-backdrop-opacity:        $modal-backdrop-opacity !default;

 $code-font-size:                    .875em !default;
 $code-color:                        $gray-700 !default;
+$code-dark-color:                   $gray-300 !default; // Boosted mod

 $kbd-padding-y:                     $spacer * .05 !default;
 $kbd-padding-x:                     $spacer * .05 !default;

@louismaximepiton
Copy link
Member Author

louismaximepiton commented Jan 7, 2022

Differents solutions I've seen so far, and I'm confused about the one to choose :

  • Adding a background to <code> and a fix color to its text. Issue: Change every code, even for example code -> not good to me.
  • Find a fixed color that goes well for black and white background color at least (#767676 seems to do it on AA level). Issue: not flexible + not AAA level.
  • Change in every component the text color for <code>. Issue: Not easy to maintain.
  • Change the color value to inherit. Issue: Hard to differenciate from the normal text.

@julien-deramond

This comment was marked as outdated.

@julien-deramond julien-deramond added this to In progress in v5.2.0 via automation Jan 12, 2022
@julien-deramond

This comment was marked as outdated.

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

As discussed in DM, the issue regarding the <code> happens too with the links (displayed black with a black bg). We should wait to identify a solution to tackle those 2 topics before merging this PR.

site/content/docs/5.1/components/accordion.md Outdated Show resolved Hide resolved
louismaximepiton and others added 4 commits February 14, 2022 08:19
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Sound good to me after the dark texts PR and some minor modifications. @louismaximepiton I let you check my last commits on this branch. If it's OK for you then I can merge it.

@julien-deramond julien-deramond merged commit 6f9f286 into main Feb 14, 2022
@julien-deramond julien-deramond deleted the main-lmp-dark-accordions branch February 14, 2022 09:19
v5.2.0 automation moved this from In progress to Done Feb 14, 2022
@julien-deramond julien-deramond mentioned this pull request Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants