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

BP-1293: Add theming support to new rich-text editor for basic redactor configuration #240

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
94a3604
BP-1293: Adding Redactor overrides in entry
jovana-marceta Oct 30, 2024
19895ee
BP-1293: Extending redactor generator mixins
jovana-marceta Oct 30, 2024
1196aa4
BP-1293: Add redactor generators for buttons and table styles
jovana-marceta Nov 5, 2024
162bab4
BP-1293: Adding redactor checkboxes generator
jovana-marceta Nov 5, 2024
0444e4e
BP-1293: Refactoring redactor styles
jovana-marceta Nov 6, 2024
71f0a1c
Use mixin for row coloring from foundation
jovana-marceta Nov 6, 2024
6f32991
BP-1293: Inherit angular material checkbox background and change alia…
Nov 8, 2024
d58d056
Rename redactor lib
jovana-marceta Nov 12, 2024
22133a9
Move non-theming related variables outside of redactor mixins
jovana-marceta Nov 12, 2024
dead72d
BP-1293: Add redactor layout generator and clean up generator file
jovana-marceta Nov 12, 2024
6768b1a
BP-1293: Inherit table even background from angular material
jovana-marceta Nov 12, 2024
80792fb
BP-1293: Inherit material body text color
jovana-marceta Nov 12, 2024
f4151f9
BP-1293: Make redactor typography generator more redeable
jovana-marceta Nov 13, 2024
b9f241c
BP-1293: Rename folder
jovana-marceta Nov 13, 2024
8d2d02d
BP-1293: Resolve review suggestions
jovana-marceta Nov 14, 2024
5a54b11
BP-1293: Get rid of variables
jovana-marceta Nov 14, 2024
194e84e
rename checkbox variables
jovana-marceta Nov 14, 2024
2bdd9ea
rename toolbar generator
jovana-marceta Nov 14, 2024
6977b0b
BP-1293: Add missing sign on buttons generator variable
jovana-marceta Nov 20, 2024
d4dda13
BP-1293: Get rid of checkboxes duplicated extraction
jovana-marceta Nov 20, 2024
0789b8f
Get rid of toolbar generator duplicated extraction retrieval
jovana-marceta Nov 20, 2024
608e968
Additional buttons and checkboxes file refactoring
jovana-marceta Nov 26, 2024
b4ea198
Make buttons generator more redeable
jovana-marceta Nov 26, 2024
2bb570a
Refactoring checkbox and toolbar generator
jovana-marceta Nov 26, 2024
2102dcc
Fixed indentation for buttons generator
jovana-marceta Nov 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions libs/entry-components/styles/_generator.scss
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
@use 'modules/components/toggle/generator' as toggle;
@use 'modules/default-theme' as default;
@use 'modules/vendors/angular-material/generator' as material-theme;
@use 'modules/vendors/rich-text/generator' as rich-text;
@use 'sass:map';
@use 'partials/theming';

Expand All @@ -23,4 +24,5 @@
@include dialogs.generate-from($merged-theme);
@include checkboxes.generate-from($merged-theme);
@include toggle.generate-from($merged-theme);
@include rich-text.generate-from($merged-theme);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
@use 'sass:map';
@use 'necessary-mix/tables-generator' as tables;
@use 'necessary-mix/buttons-generator' as buttons;
@use 'necessary-mix/typography-generator' as typography;
@use 'necessary-mix/checkboxes-generator' as checkboxes;
@use 'necessary-mix/layout-generator' as layout;

@mixin generate-from($theme) {
@include typography.generate-from($theme);
@include tables.generate-from($theme);
@include buttons.generate-from($theme);
@include checkboxes.generate-from($theme);
@include layout.generate-from($theme);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
@use 'sass:map';
@use '@angular/material' as mat;

$default-material-button: map.get(mat.define-typography-config(), 'button');

@mixin generate-from($theme) {
$theming-buttons: map.get($theme, 'general', 'fonts', 'buttons');

.enigmatry-redactor-content {
button, .rx-content button {

@if (theming-buttons, 'colors', 'primary') {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not aware of this comparison. Can you give us as some example of docs from which you learned this?

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 made a mistake and forgot to add the $ sign to the theming-buttons variable. Condition should look like this:
@if (map.get($theme, 'general', 'fonts', 'buttons'), 'colors', 'primary')

Copy link
Member

Choose a reason for hiding this comment

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

That's fine. Short and sweat, but where did you find it? Nothing about that in official docs AFAIK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ability to pass multiple keys to map.get is a feature supported in Dart Sass, as described in the official Sass documentation. This functionality allows for concise access to deeply nested values in maps without chaining multiple map.get calls manually. It simplifies the process of working with complex nested data structures like our $theme map

This feature is documented in the official Sass sass:map module

Additionally, here are GitHub issuse tracking the feature:
sass/sass#1739
sass/dart-sass#1884

Copy link
Member

Choose a reason for hiding this comment

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

This is not about map.get keys. We're using that on various places. The question was about if condition joined with such map syntax?
IDK how this would even compile: @if ($theming-buttons, 'colors', 'primary')

Is the intention to map.get primary color and then compare it with unset (null) value?

background-color: map.get($theme, 'general', 'colors', 'primary');
}
@else {
background-color: mat.get-color-from-palette(mat.$primary-palette, 500);
}

@if (theming-buttons, 'family') and map.get($theming-buttons, 'family') != '' {
font-family: map.get($theming-buttons, 'family');
}
@else {
font-family: map.get($default-material-button, 'font-family');
font-weight: map.get($default-material-button, 'font-weight');
}

@if (theming-buttons, 'size') {
font-size: map.get($theming-buttons, 'size');
}
@else {
font-size: map.get($default-material-button, 'font-size');
}
}

.rx-toolbar-buttons .rx-button {
&.active, &:hover {
@if map.get($theme, 'general', 'inputs', 'background') {
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicated extraction. We can just do it once and make null comparison if unset.

background: map.get($theme, 'general', 'inputs', 'background');
}
@else {
background: transparent;
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
@use 'sass:map';
@use '@angular/material' as mat;

$redactor-checkboxes-selector: '[data-rx-type = 'todoitem'] ';
$default-material-checkbox: mat.get-color-from-palette(mat.define-palette(mat.$pink-palette), 500);
jovana-marceta marked this conversation as resolved.
Show resolved Hide resolved

@mixin generate-from($theme) {

$theming-checkboxes: map.get($theme, 'general', 'checkboxes');

.enigmatry-redactor-content {

@if map.get($theme, 'general', 'colors', 'accent') {
jovana-marceta marked this conversation as resolved.
Show resolved Hide resolved
.rx-editor #{$redactor-checkboxes-selector} input:checked::before {
border-color: map.get($theme, 'general', 'colors', 'accent');
background-color: map.get($theme, 'general', 'colors', 'accent');
}
}
@else {
.rx-editor #{$redactor-checkboxes-selector} input:checked::before {
border-color: $default-material-checkbox;
background-color: $default-material-checkbox;
}
}

@if $theming-checkboxes {
.rx-editor #{$redactor-checkboxes-selector} input::before {
background-color: map.get($theming-checkboxes, 'background');
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
@use 'sass:map';

@mixin generate-from($theme) {
.enigmatry-redactor-content {
.rx-main-container, .rx-toolbox-container {
@if map.get($theme, 'general', 'inputs', 'background') {
background: map.get($theme, 'general', 'inputs', 'background');
}
@else {
background: transparent;
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
@use 'sass:map';
@use '@angular/material' as mat;
@use 'scss-foundation/src/modules/lists/row-coloring' as list;

$redactor-th-selector: 'thead th';
$body-typography: map.get(mat.define-typography-config(), 'body-1');
$caption-typography: map.get(mat.define-typography-config(), 'caption');

@mixin generate-from($theme) {
$theming-table: map.get($theme, 'tables', 'rows', 'odd-even-background');
$theming-fonts: map.get($theme, 'general', 'fonts');

.enigmatry-redactor-content {
table {
@if $theming-table {
tr {
@include list.odd-row-coloring(map.get($theming-table, 'odd'));
@include list.even-row-coloring(map.get($theming-table, 'even'));
}
}
@else {
tr {
@include list.even-row-coloring(mat.get-color-from-palette(mat.$grey-palette, 100));
typhoon41 marked this conversation as resolved.
Show resolved Hide resolved
@include list.odd-row-coloring(mat.get-color-from-palette(mat.$grey-palette, 50));
}
}

@if (map.get($theme, 'tables', 'header', 'font-size')) {
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated retrieval.

th {
font-size: map.get($theme, 'tables', 'header', 'font-size');
}
}
}

table, table td, table th {
/* stylelint-disable-next-line scss/at-if-no-null */
@if $theming-fonts and map.has-key($theming-fonts, 'body') and map.get($theming-fonts, 'body', 'size') != null {
Copy link
Member

Choose a reason for hiding this comment

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

Will it fail compiling if we just check directly size from body (without has key and previous check)?
We would then get rid of duplicated retrieval.

Copy link
Member

Choose a reason for hiding this comment

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

Still legit.

font-size: map.get($theming-fonts, 'body', 'size');
}
@else {
font-size: map.get($body-typography, 'font-size');
}

#{$redactor-th-selector} {
/* stylelint-disable-next-line scss/at-if-no-null */
@if map.has-key($theme, 'tables') and map.has-key(map.get($theme, 'tables', 'header'), 'font-size')
Copy link
Member

Choose a reason for hiding this comment

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

The same applies for this one.

and map.get($theme, 'tables', 'header', 'font-size') != null {
font-size: map.get($theme, 'tables', 'header', 'font-size');
}
@else {
font-size: map.get($caption-typography, 'font-size');
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/* stylelint-disable selector-max-compound-selectors, scss/at-if-no-null */
@use 'sass:map';
@use '@angular/material' as mat;

$body-typography: map.get(mat.define-typography-config(), 'body-1');
$hero-typography: map.get(mat.define-typography-config(), 'headline-1');
$title-typography: map.get(mat.define-typography-config(), 'subtitle-1');
$caption-typography: map.get(mat.define-typography-config(), 'caption');

@mixin generate-from($theme) {
$theming-fonts: map.get($theme, 'general', 'fonts');

.enigmatry-redactor-content {
.rx-editor, .rx-content, .rx-content p {
@if $theming-fonts and map.has-key($theming-fonts, 'body') and map.get($theming-fonts, 'body', 'color') != null {
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 attempted various approaches to avoid the null value and bypass the stylelint rule, but unfortunately, none of them were successful.

Copy link
Member

Choose a reason for hiding this comment

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

map.get($theming-fonts, 'body', 'color') != null fails to compile without previous checks if i.e. $theming-font is unset or there is no body defined in it?

color: map.get($theming-fonts, 'body', 'color');
}
@else {
color: map.get($body-typography, 'color');
}

@if $theming-fonts and map.has-key($theming-fonts, 'body') and map.get($theming-fonts, 'body', 'family') != '' {
font-family: map.get($theming-fonts, 'body', 'family');
}
@else {
font-family: map.get($body-typography, 'font-family');
}

Copy link
Member

Choose a reason for hiding this comment

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

This is too much duplication and hard to follow. Maybe an option to change this is to replace with the following:

  • pseudocode -
    Get the body.
    Check if body is null, if it is, created default settings and return.
    If it's not, try getting individual settings, the way it's already done, but without constant and the same null checks + syntax will be way shorter once you have body,

@if $theming-fonts and map.has-key($theming-fonts, 'body') and map.get($theming-fonts, 'body', 'size') != null {
font-size: map.get($theming-fonts, 'body', 'size');
}
@else {
font-size: map.get($body-typography, 'font-size');
}

@if $theming-fonts and map.has-key($theming-fonts, 'body') and map.get($theming-fonts, 'body', 'letter-spacing') != null {
letter-spacing: map.get($theming-fonts, 'body', 'letter-spacing');
}
@else {
letter-spacing: map.get($body-typography, 'letter-spacing');
}

/* stylelint-disable-next-line selector-max-combinators */
h1, h2, h3, h4 {
@if $theming-fonts and map.has-key($theming-fonts, 'hero-titles') and map.get($theming-fonts, 'hero-titles', 'family') != '' {
font-family: map.get($theming-fonts, 'hero-titles', 'family');
}
@else {
font-family: map.get($hero-typography, 'font-family');
}

@if $theming-fonts and map.has-key($theming-fonts, 'hero-titles') and map.get($theming-fonts, 'hero-titles', 'letter-spacing') != null {
letter-spacing: map.get($theming-fonts, 'hero-titles', 'letter-spacing');
}
@else {
letter-spacing: map.get($hero-typography, 'letter-spacing');
}
}

/* stylelint-disable-next-line selector-max-combinators */
h5, h6 {
@if $theming-fonts and map.has-key($theming-fonts, 'titles') and map.get($theming-fonts, 'titles', 'family') != '' {
font-family: map.get($theming-fonts, 'titles', 'family');
}
@else {
font-family: map.get($title-typography, 'font-family');
}

@if $theming-fonts and map.has-key($theming-fonts, 'titles') and map.get($theming-fonts, 'titles', 'letter-spacing') != null {
letter-spacing: map.get($theming-fonts, 'titles', 'letter-spacing');
}
@else {
letter-spacing: map.get($title-typography, 'letter-spacing');
}
}
}
}
}
1 change: 1 addition & 0 deletions libs/entry-components/styles/partials/theming.scss
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
@use 'core/components';
@use 'vendors/overrides';
jovana-marceta marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions libs/entry-components/styles/partials/vendors/_index.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@use 'overrides/rich-text';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@use 'rich-text';
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
@use 'scss-foundation/src/modules/display/items';
@use '@angular/material' as mat;

$redactor-link-selectors: 'a, a:visited, a:focus, a:hover';

.enigmatry-redactor-content .rx-content {
#{$redactor-link-selectors} {
@include items.fully-align(center, center);
display: inline-flex;
cursor: pointer;
}

button, #{$redactor-link-selectors} {
@include items.fully-align(center, flex-start);
height: 36px;
margin: 8px 8px 8px 0;
padding: 0 16px;
border: none;
text-decoration: none;
}

a:hover {
background-color: mat.get-color-from-palette(mat.$grey-palette, 100);
}

button {
@include mat.elevation(4);
jovana-marceta marked this conversation as resolved.
Show resolved Hide resolved
height: 36px;
border-radius: 4px;
color: #FFF;

&:hover {
@include mat.elevation(8);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
@use '@angular/material' as mat;

$checkbox-width: 18px;
jovana-marceta marked this conversation as resolved.
Show resolved Hide resolved
$checkbox-height: 18px;
$border-radius: 2px;
$redactor-todoitem-selector: '.rx-editor [data-rx-type = 'todoitem']';

.enigmatry-redactor-content #{$redactor-todoitem-selector} {
input {
width: $checkbox-width;
height: $checkbox-height;
}

input::before, input:checked::before {
width: $checkbox-width;
height: $checkbox-height;
border-radius: $border-radius;
border-color: rgb(0 0 0 / .54);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
@use 'sass:map';
@use '@angular/material' as mat;

.enigmatry-redactor-content {
.rx-button-icon svg {
fill: rgb(0 0 0 / .54);
}

.rx-main-container {
border-color: rgb(0 0 0 / .38);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@use 'general';
@use 'tables';
@use 'buttons';
@use 'forms';
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/* stylelint-disable selector-max-compound-selectors, selector-max-type, selector-max-combinators */
.enigmatry-redactor-content table {
th {
font-weight: 500;
}

p {
margin: 0;
}

td {
border: 1px solid rgb(0 0 0 / .12);
}

tr:first-child {
th, td {
border-top: none;
}
}

tr:last-child td {
border-bottom: none;
}

th, td {
border: {
right: none;
left: none;
}
}
}