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

Rich text updates #1625

Closed
wants to merge 1 commit into from
Closed

Rich text updates #1625

wants to merge 1 commit into from

Conversation

andrewetchen
Copy link
Contributor

Draft PR

Rich text updates - UX feedback

cc: @wiktoriaswiecicka

Why are these changes introduced?

Related: #1460

What approach did you take?

Section level settings

  • Desktop context position
  • Text/content alignment

Block level settings and additions

  • Add a new "Extra large" heading option
  • Change the heading type from text to richtext to allow merchants to use bold, italic, links, dynamic source, and underline
  • Add a caption block
  • Remove limit for each block

Other considerations

Range slider widths will need to be explored further / section parity etc.

Demo links


Questions

Section settings

  • Text position on desktop:
    • This setting currently doesn't exist in our theme. The only similar setting that does is "Desktop content position" which I think conveys the same meaning. Let me know your thoughts on this.
  • Text or content alignment:
    • Most other sections have "Desktop content alignment" and "Mobile content alignment".
    • We also have a global "Text alignment" setting for Cards.
    • Since this setting will set the alignment of all block elements, I think it makes sense to go with something that includes "content" instead of "text". If this ends up being the case:
      • Should we keep "Content alignment"?
      • For section parity, the Image with text section has "Desktop content alignment" and "Mobile content alignment" - if there are plans to include a mobile adjustment for text/content alignment, would it make more sense to offer "Desktop content alignment" and "Mobile content alignment" for rich text instead of just one text/content alignment adjustment for desktop and mobile. Thoughts?

Extra large heading size

  • Let me know if this needs to be made smaller or larger.

Caption block

  • I used the same caption block from the Image with text section which includes text style and text size. Let me know if this is okay or if I should remove either text style or text size.
  • The default language for the caption block is "Add a tagline".
    • Should we change this to something else like, "Add a caption" or "Caption"?

Block limit removal

  • I've removed the block limit for each block: Heading, Text, Caption, and Button. Looking for feedback on whether we should keep the limit for certain blocks and if so, which ones? I'm perfectly okay with the way it is now but I just think having multiple button blocks might cause confusion or increased cognitive load for buyers. My mental model is one button being associated per rich text section.

Presets

  • The current presets when adding a new Rich text section includes one of each: Heading, Text, Button
    • Should we include the new caption block?

@melissaperreault melissaperreault requested review from melissaperreault and removed request for melissaperreault April 15, 2022 16:29
Comment on lines +566 to +569
.left {
text-align: left;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although this isn't required (text-align: left;) being the default value, it might be used for other elements and for instances where we'd need to override an existing text-align value.

{%- for block in section.blocks -%}
{%- case block.type -%}
{%- when 'heading' -%}
<h2 class="rich-text__heading rte {{ block.settings.heading_size }}" {{ block.shopify_attributes }}>{{ block.settings.heading }}</h2>
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'll need to update the h2 so that it doesn't include <p> tags for the richtext, to prevent this:

Screenshot

image

Either of the following works but I'll need to confirm if its the solution we want to go with for rich text headings:

<h2 class="rich-text__heading rte {{ block.settings.heading_size }}" {{ block.shopify_attributes }}>{{ block.settings.heading | replace: '<p>', '<span>' | replace: '</p>', '</span>' }}</h2>

Or

<h2 class="rich-text__heading rte {{ block.settings.heading_size }}" {{ block.shopify_attributes }}>{{ block.settings.heading | replace: 'p>', 'span>' }}</h2>
Screenshot

image

Confirmed with: https://validator.w3.org/nu/#textarea

Comment on lines +34 to +40
{%- if block.settings.button_label != blank -%}
<div class="rich-text__button">
<a{% if block.settings.button_link == blank %} role="link" aria-disabled="true"{% else %} href="{{ block.settings.button_link }}"{% endif %} class="button{% if block.settings.button_style_secondary %} button--secondary{% else %} button--primary{% endif %}" {{ block.shopify_attributes }}>
{{ block.settings.button_label | escape }}
</a>
</div>
{%- endif -%}
Copy link
Contributor Author

@andrewetchen andrewetchen Apr 18, 2022

Choose a reason for hiding this comment

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

The {%- if block.settings.button_label != blank -%} conditional was added to prevent this: #1615.

Additionally, I added a <div> to make sure button blocks are placed on new lines instead of inline.

@@ -53,7 +53,7 @@
"heading": {
"type": "heading",
"settings": {
"heading": "Talk about your brand",
"heading": "<p>Talk about your brand</p>",
Copy link
Contributor Author

@andrewetchen andrewetchen Apr 18, 2022

Choose a reason for hiding this comment

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

Related: https://github.com/Shopify/dawn/pull/1625/files#r852183418

richtext input setting

p tags must be used to wrap the default richtext. It will result in an error if it's not:

shopify-cli

11:12:21 ERROR  » update templates/index.json:
  Setting 'heading' is invalid. All top level nodes must be '<p>' tags

Code editor

image

@bakura10
Copy link

The ability to have a setting to set up a position (top left, top top, top right, left middle…) is very very common for many sections, and having this as a select cause lot of duplication of wording.

i think this may be a new built in setting that Shopify could add, with nice visual icons to convey the different position. We (theme developers) should be able to exclude some choices.

this setting type could also be used for text positioning (left, venter, right).

what do you think ? :)

@andrewetchen
Copy link
Contributor Author

andrewetchen commented Apr 22, 2022

Hey @bakura10,

Regarding the extra positional settings, this would definitely add more flexibility and similarity between some of our sections but unfortunately, I don't think it will be added to the rich text section, at least not for this iteration. I will record your feedback/suggestion - thanks so much!

@andrewetchen andrewetchen mentioned this pull request Apr 22, 2022
10 tasks
@andrewetchen andrewetchen deleted the rich-text-draft-2 branch April 22, 2022 22:01
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