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

[BD-46] docs: replaced SCSS variables with CSS variables on the Colors page #2376

Merged

Conversation

PKulkoRaccoonGang
Copy link
Contributor

@PKulkoRaccoonGang PKulkoRaccoonGang commented Jun 14, 2023

Description

  • refactored Color page;
  • added CSS variables.

Github issue

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 14, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jun 14, 2023

Thanks for the pull request, @PKulkoRaccoonGang!

When this pull request is ready, tag your edX technical lead.

@PKulkoRaccoonGang PKulkoRaccoonGang changed the title docs: replaced SCSS variables with CSS variables on the Colors page o… [BD-46] docs: replaced SCSS variables with CSS variables on the Colors page Jun 14, 2023
@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program and removed open-source-contribution PR author is not from Axim or 2U labels Jun 14, 2023
@PKulkoRaccoonGang PKulkoRaccoonGang self-assigned this Jun 14, 2023
@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (09570d7) 91.95% compared to head (12a2267) 91.95%.
Report is 1 commits behind head on alpha.

Additional details and impacted files
@@           Coverage Diff           @@
##            alpha    #2376   +/-   ##
=======================================
  Coverage   91.95%   91.95%           
=======================================
  Files         216      216           
  Lines        3618     3618           
  Branches      891      891           
=======================================
  Hits         3327     3327           
  Misses        286      286           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@viktorrusakov
Copy link
Contributor

@PKulkoRaccoonGang I think that code could be greatly simplified on this page since we use CSS variables now. Previously we used MeasuredItem component to render a div with specific className to the get div's color with browser API and display its value. This is redundant now since we know in advance which CSS variable is used to render each color, so we can get color value just by variable's name. E.g., Swatch component could be rewritten like this

const styles = getComputedStyle(document.body);

function Swatch({ name, colorClassName, isUnused }: ISwatch) {
  const computedValue = styles.getPropertyValue(name);
  return (
    <div className="d-flex align-items-center mb-2">
      <div
        className={classNames('p-3 mr-2 rounded', colorClassName, {
          'unused-level': isUnused,
        })}
      />
      <div style={{ lineHeight: 1 }} className="small">
        <code className="mb-0 d-block text-lowercase text-dark-700">
          {`var(${name})`}
        </code>
        <code style={{ fontSize: '65%' }} className="text-muted">
          {computedValue}
        </code>
      </div>
    </div>
  );
}

// usage example, note that you need to pass var's name to the "name" prop without wrapping it in 'var'
<Swatch name="--pgn-color-accent-a" colorClassName="bg-accent-a" />

I believe this lets you completely remove usages of MeasuredItem and Color components from this page and this is a good thing 🙂
Works for me at least
image

Also, there's something wrong with the borders
image

they render with same styles for some reason, although they should have different colors. I'll let you figure this one out on your own 😛

@netlify
Copy link

netlify bot commented Jul 20, 2023

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 12a2267
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/650305f1b530bc00081cd205
😎 Deploy Preview https://deploy-preview-2376--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@viktorrusakov
Copy link
Contributor

@PKulkoRaccoonGang I noticed you pushed a commit with my suggestions, but I still see issues with borders that I described above. Just reminding you in case you missed or forgot about that part. If you just didn't have time to look at that yet, that's ok 🙂

Also, deploy preview is failing now because you used getComputedStyle which is not defined during build. Probably need to check if it's defined before using it.

@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as draft July 20, 2023 20:15
Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

Left 1 comment alongside Viktor's feedback.

The only high-level feedback about the page I have might be about whether we can have the "accents" colors shown as the last color variants in the UI given there's only 2 of them (throws off the visual grid haha) and are generally less commonly used (non-blocking, can be deferred).

www/src/pages/foundations/colors.tsx Outdated Show resolved Hide resolved
@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as ready for review September 8, 2023 10:20
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/update-color-page branch 3 times, most recently from 845c058 to 3959934 Compare September 8, 2023 11:08
@viktorrusakov viktorrusakov merged commit bf05cc3 into openedx:alpha Sep 15, 2023
9 checks passed
@openedx-webhooks
Copy link

@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-semantic-release
Copy link
Contributor

🎉 This PR is included in version 22.0.0-alpha.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 23.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

PKulkoRaccoonGang added a commit that referenced this pull request Aug 1, 2024
…2376)

* docs: replaced SCSS variables with CSS variables on the Colors page of the documentation site

* refactor: refactoring after review

* refactor: refactoring after review

* fix: removed extra commons.css generation

* refactor: code refactoring

* refactor: removed Colors package
PKulkoRaccoonGang added a commit that referenced this pull request Aug 4, 2024
…2376)

* docs: replaced SCSS variables with CSS variables on the Colors page of the documentation site

* refactor: refactoring after review

* refactor: refactoring after review

* fix: removed extra commons.css generation

* refactor: code refactoring

* refactor: removed Colors package
PKulkoRaccoonGang added a commit that referenced this pull request Aug 4, 2024
…2376)

* docs: replaced SCSS variables with CSS variables on the Colors page of the documentation site

* refactor: refactoring after review

* refactor: refactoring after review

* fix: removed extra commons.css generation

* refactor: code refactoring

* refactor: removed Colors package
PKulkoRaccoonGang added a commit that referenced this pull request Aug 5, 2024
…2376)

* docs: replaced SCSS variables with CSS variables on the Colors page of the documentation site

* refactor: refactoring after review

* refactor: refactoring after review

* fix: removed extra commons.css generation

* refactor: code refactoring

* refactor: removed Colors package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blended PR is managed through 2U's blended developmnt program raccoon-gang released on @alpha
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

Update "Colors" page in Paragon docs site to reflect design tokens and CSS variables
7 participants