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

feat: introduce additional CKEditor theming #5

Merged
merged 8 commits into from
Feb 1, 2021
Merged

Conversation

austingreendev
Copy link
Contributor

@austingreendev austingreendev commented Jan 19, 2021

Description

This PR adds additional CKEditor theming. Its scope is limited to basic styling and doesn't attempt to override any layout, spacing, or advanced functionality at this time.

Detail

Some core areas to focus on are:

  • Heading dropdown menu now includes Garden selected/hover/focus styling (not updates to menu positioning yet)
  • Disabled heading dropdown now aligns with IconButton styling
  • The typearound widget now includes vertical before/after cursors instead of the large the default horizontal implementations.
    • This includes new RTL logic for before/after
  • The horizontal rules styling is updated including the margin
  • The widget focus styling now mimics the default Garden focus ring. I'm unable to modify the border-radius as this time since inline widgets since that is determined by the individual widget. I will look into this more.
  • Override code selected background color shift
  • Move inline code to be aligned with bold, italic, underline
  • Customize alt+F10 focus states for icon buttons and list focus state.

@zendesk-garden zendesk-garden temporarily deployed to staging January 19, 2021 19:17 Inactive
@zendesk-garden zendesk-garden temporarily deployed to staging January 19, 2021 20:04 Inactive
@zendesk-garden zendesk-garden temporarily deployed to staging January 21, 2021 18:22 Inactive
@zendesk-garden zendesk-garden temporarily deployed to staging January 25, 2021 22:22 Inactive
@austingreendev austingreendev changed the title chore: add dropdown menu theming feat: introduce additional CKEditor theming Jan 27, 2021
@zendesk-garden zendesk-garden temporarily deployed to staging January 27, 2021 01:04 Inactive
@austingreendev austingreendev requested review from m-lai and a team January 27, 2021 01:07
@austingreendev austingreendev marked this pull request as ready for review January 27, 2021 01:07
@m-lai
Copy link

m-lai commented Jan 27, 2021

@austingreendev Based on your listed changes, all looks kosher to me! The only thing I'm wondering about is whether the chevron in the Heading dropdown menu is vertically center-aligned with the Heading text, it looks slightly off center.

@austingreendev
Copy link
Contributor Author

Agreed. I'll take a look at the vertical spacing there.

@zendesk-garden zendesk-garden temporarily deployed to staging January 27, 2021 22:50 Inactive
Copy link

@hzhu hzhu left a comment

Choose a reason for hiding this comment

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

Had a few questions but LGTM!

src/stories/theme/_list.css Show resolved Hide resolved
src/stories/plugins/CodeBlockUI.js Show resolved Hide resolved
src/stories/theme/_button.css Outdated Show resolved Hide resolved
src/stories/theme/_dropdown.css Outdated Show resolved Hide resolved
@@ -1,3 +1,5 @@
.ck-content hr {
margin: var(--ck-spacing-unit) 0;
margin: var(--ck-spacing-extra-large) 0;
Copy link
Member

Choose a reason for hiding this comment

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

There might need to be a notion of padding if border-radius focus is to be applied to this element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the horizontal line spacing to match the expected focus-ring size. I'll update any required padding/margin when we handle all of the content spacing in a following PR.

src/stories/theme/_list.css Show resolved Hide resolved
src/stories/theme/_typearound.css Outdated Show resolved Hide resolved
src/stories/theme/_dropdown.css Outdated Show resolved Hide resolved
src/stories/theme/_button.css Outdated Show resolved Hide resolved
@zendesk-garden zendesk-garden temporarily deployed to staging January 29, 2021 19:56 Inactive
@zendesk-garden zendesk-garden temporarily deployed to staging January 29, 2021 20:24 Inactive
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

😍

@zendesk-garden zendesk-garden temporarily deployed to staging January 29, 2021 21:15 Inactive
@austingreendev austingreendev merged commit 9581cde into main Feb 1, 2021
@austingreendev austingreendev deleted the agreen/theming branch February 1, 2021 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants