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

Fix some missed updates in theming #12154

Merged
merged 2 commits into from
May 13, 2024

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented May 9, 2024

Summary

References

Unreported regressions.

Reviewer guidance

Example of one place this makes a difference:
Before
Screenshot from 2024-05-09 16-46-56

After
Screenshot from 2024-05-09 16-48-26


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

🚀 This description was created by Ellipsis for commit e239cbe

Summary:

This PR updates color validation and references across multiple components to align with new color naming conventions in the design system.

Key points:

  • Updated color validation in themeSpec.js to new color names.
  • Changed color references in DeprecationWarningBanner.vue, DeviceSettingsPage/index.vue, and UnPinnedDevices.vue to align with updated color names.

Generated with ❤️ by ellipsis.dev

@rtibbles rtibbles added the TODO: needs review Waiting for review label May 9, 2024
@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend labels May 9, 2024
@rtibbles rtibbles added this to the Kolibri 0.16: Planned Patch 2 milestone May 9, 2024
@rtibbles rtibbles requested a review from MisRob May 10, 2024 00:00
@MisRob
Copy link
Member

MisRob commented May 10, 2024

Hey @ellipsis, give me a code review

Copy link

ellipsis-dev bot commented May 10, 2024

OK! Reviewing this PR...


Responding to this comment by @MisRob. For more information about Ellipsis, check the documentation.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to e239cbe in 1 minute and 36 seconds

More details
  • Looked at 88 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. kolibri/core/assets/src/styles/themeSpec.js:11
  • Draft comment:
    The updated color names in _brandColorScaleValidator function are consistent with the new expected color names provided in the PR description. This change ensures that the theme validator checks for the correct color names.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR description mentions updating the color token references to match the new expected values. The changes in the files reflect updates to the color references in various Vue components. The changes are consistent with the new color tokens provided in the linked color reference files. The PR seems to be focused on updating these references to ensure the UI components use the correct theming colors.
2. kolibri/plugins/device/assets/src/views/DeprecationWarningBanner.vue:6
  • Draft comment:
    The update to use $themePalette.yellow.v_200 for the background color aligns with the new theming guidelines. This ensures the deprecation warning banner uses the correct color as per the updated theme.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in the DeprecationWarningBanner.vue file updates the background color of the warning banner to use the new color token $themePalette.yellow.v_200. This change is consistent with the PR's intent to update color references according to the new theming guidelines.
3. kolibri/plugins/device/assets/src/views/DeviceSettingsPage/index.vue:484
  • Draft comment:
    The update to use $themePalette.grey.v_600 for the information description color aligns with the new theming guidelines. This ensures that the text color in the device settings page uses the correct color as per the updated theme.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in the DeviceSettingsPage/index.vue file updates the color of the information description text to use the new color token $themePalette.grey.v_600. This change is consistent with the PR's intent to update color references according to the new theming guidelines.
4. kolibri/plugins/learn/assets/src/views/LibraryPage/UnPinnedDevices.vue:114
  • Draft comment:
    The update to use $themePalette.grey.v_600 for the channel text color aligns with the new theming guidelines. This ensures that the text color in the unpinned devices component uses the correct color as per the updated theme.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in the UnPinnedDevices.vue file updates the color of the channel text to use the new color token $themePalette.grey.v_600. This change is consistent with the PR's intent to update color references according to the new theming guidelines.

Workflow ID: wflow_nC361pj1xc4zD5hV


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

6 days left in your free trial, upgrade for $20/seat/month or contact us.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

looks good to me

@marcellamaki marcellamaki merged commit f5e9d6d into learningequality:release-v0.16.x May 13, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants