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

chore: Convert word cloud block sass variables to css variables #35386

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

farhan
Copy link
Contributor

@farhan farhan commented Aug 28, 2024

Ticket: #35306

Description:
Convert word cloud block sass variables to css variables

Other Relevant PR's:
#35233
#35385

How to test:

  • Add the word cloud block in a test course
  • Open it in the LMS
  • Make sure following property is accessible
    getComputedStyle(document.documentElement).getPropertyValue('--baseline')

Pending/Known Issue:

5px or appropriate value of margin should be reflected in the styling of word-cloud xblock unit frame but it's not.
I think the issue is .xmodule_display.xmodule_WordCloudBlock class doesn't exist referred in the generatd css file given below (css generated on the master branch for the testing)

@import url("https://fonts.googleapis.com/css?family=Open+Sans:300,400,400i,600,700");
/* line 1, /openedx/edx-platform/xmodule/assets/WordCloudBlockDisplay.scss */
.xmodule_display.xmodule_WordCloudBlock {
  /* stylelint-disable-line */
  /* stylelint-disable-line */ }
  /* line 6, /openedx/edx-platform/xmodule/assets/word_cloud/_display.scss */
  .xmodule_display.xmodule_WordCloudBlock .input-cloud {
    margin: 5px; }
  /* line 10, /openedx/edx-platform/xmodule/assets/word_cloud/_display.scss */
  .xmodule_display.xmodule_WordCloudBlock .result_cloud_section {
    display: none;
    width: 0px;
    height: 0px; }
  /* line 16, /openedx/edx-platform/xmodule/assets/word_cloud/_display.scss */
  .xmodule_display.xmodule_WordCloudBlock .result_cloud_section.active {
    display: block;
    width: 100%;
    height: auto;
    margin-top: 1em; }
    /* line 22, /openedx/edx-platform/xmodule/assets/word_cloud/_display.scss */
    .xmodule_display.xmodule_WordCloudBlock .result_cloud_section.active h3 {
      font-size: 100%; }
  /* line 27, /openedx/edx-platform/xmodule/assets/word_cloud/_display.scss */
  .xmodule_display.xmodule_WordCloudBlock .your_words {
    font-size: 0.85em;
    display: block; }

@farhan farhan requested review from feanil, kdmccormick and irtazaakram and removed request for irtazaakram August 28, 2024 11:02
@kdmccormick
Copy link
Member

@farhan does the styling issue happen without your PR as well, or is it triggered by your PR?

@farhan
Copy link
Contributor Author

farhan commented Aug 29, 2024

@farhan does the styling issue happen without your PR as well, or is it triggered by your PR?

@kdmccormick Issue exists in the master branch, not triggered with my PR.
I was planning to merge this PR after testing but in b/w I surfaced this issue.

@farhan
Copy link
Contributor Author

farhan commented Aug 29, 2024

As per decision with @kdmccormick let's merge this PR as issue is not triggered by this PR.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

we talked in our sync-up and decided that since the issue exists on master and since Farhan was able to confirm that the new css variable shows up in the DOM, that we do not to block this PR on a styling fix

@farhan farhan merged commit 7530927 into master Aug 29, 2024
49 checks passed
@farhan farhan deleted the farhan/convert-wordcloud-sass-to-css branch August 29, 2024 13:19
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been rolled back from the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants