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

Update infobox styling #406

Merged
merged 6 commits into from
May 13, 2024
Merged

Update infobox styling #406

merged 6 commits into from
May 13, 2024

Conversation

yepzdk
Copy link
Contributor

@yepzdk yepzdk commented May 8, 2024

@yepzdk yepzdk marked this pull request as ready for review May 8, 2024 13:35
@yepzdk yepzdk changed the title 1340: Update infobox styling Update infobox styling May 8, 2024
Copy link
Contributor

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

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

I'm not sure using a taxonomy for info box variant is the best way to go.

When using a taxonomy, the editor must make sure that the taxonomy term names match the defined info box css styles to make it work, i.e. they must know something about what goes on behind the scenes. Furthermore, using a taxonomy may make the editor think that they can just invent new info box variants by creating a new taxonomy term, and while this is technically true, it will not work in practice because there is no styling to match the new variant.

Using a simple select element on the info box paragraph may be a better solution. This will let the designer control which info box variants are available and create the matching styling if new ones are needed.

@yepzdk
Copy link
Contributor Author

yepzdk commented May 13, 2024

Thank you @rimi-itk - That makes sense. I will have a look at using a select instead.

@yepzdk yepzdk requested a review from rimi-itk May 13, 2024 08:11
Copy link
Contributor

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

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

Approved with a comment and a question?

@@ -41,7 +41,7 @@
<div class="container">
<div class="row">
<div class="col-md-8">
<div class="info-box rounded overflow-hidden {{ elements.field_variant.0['#plain_text']|clean_class }}">
<div class="info-box rounded overflow-hidden {{ elements.field_variant_select.0['#markup']|clean_class }}">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can value be used here, i.e.

Suggested change
<div class="info-box rounded overflow-hidden {{ elements.field_variant_select.0['#markup']|clean_class }}">
<div class="info-box rounded overflow-hidden {{ elements.field_variant_select.value|clean_class }}">

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would love to use value since it is cleaner. But unfortunately, it only returns null.

@@ -5,7 +5,7 @@ dependencies:
config:
- field.field.paragraph.info_box.field_content_block_text
- field.field.paragraph.info_box.field_paragraph_title
- field.field.paragraph.info_box.field_variant
- field.field.paragraph.info_box.field_variant_select
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth not including _select in the field machine name. It's not important (to not include it), but we usually don't embed the field type in the name.

Renaming the machine name can be done with search and replace in the config files and by renaming files with field_variant_select in the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely - Changed with latest commit.

@yepzdk yepzdk merged commit 05553f1 into develop May 13, 2024
12 checks passed
@yepzdk yepzdk deleted the feature/1340-infobox-design branch May 13, 2024 09:26
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.

2 participants