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

[CIVIC-1937] Add page level styles into Drupal theme. #1308

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

alan-cole
Copy link
Collaborator

@alan-cole alan-cole commented Oct 4, 2024

civictheme/uikit#392

Checklist before requesting a review

  • I have formatted the subject to include ticket number as Issue #123456 by drupal_org_username: Issue title
  • I have added a link to the issue tracker
  • I have provided information in Changed section about WHY something was done if this was not a normal implementation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have run new and existing relevant tests locally with my changes, and they passed
  • I have provided screenshots, where applicable

Changed

  1. A companion to [#392] Inline CSS dependencies. uikit#402 . Moves the page level styling into Drupal theme.

Screenshots

@alan-cole alan-cole requested a review from richardgaunt October 4, 2024 04:53
@alan-cole alan-cole self-assigned this Oct 4, 2024
@alan-cole alan-cole added the State: Needs review Pull requests needs a review from assigned developers label Oct 4, 2024
@AlexSkrypnyk
Copy link
Contributor

Hi @alan-cole! Could you please explain why these cannot go to the UIKit PR (which looks really good). Is there some special case for these elements?

@alan-cole
Copy link
Collaborator Author

Thanks @AlexSkrypnyk

My thinking is that everything in the UI Kit (except for row and col) is namespaced to CT and applies only to CT elements. In the case of html and body these styles sit outside the CT namespacing and affect elements outside the UI Kit.

As UI Kit has gone to great lengths to scope it's styles, it seems the appropriate solution would be to either have these styles be applied outside the UI Kit (e.g. in the drupal theme), or to have them scoped as well (e.g. ct-html, ct-body) to keep consistency with the other styles.

Happy to hear thoughts or comments.

@alan-cole alan-cole force-pushed the feature/CIVIC-1937-inline-css-dependencies branch from 20a04b7 to d1a7cac Compare October 7, 2024 22:37
@richardgaunt richardgaunt merged commit e618cf3 into develop Oct 7, 2024
13 checks passed
@richardgaunt richardgaunt deleted the feature/CIVIC-1937-inline-css-dependencies branch October 7, 2024 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Needs review Pull requests needs a review from assigned developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants