Blockquote Element and Elements are now top level #1088
Conversation
|
PRbuilds results: LightHouse Reporting --automated message |
src/model/unwrapHtml.ts
Outdated
| export const unwrapHtml = ( | ||
| prefix: string, | ||
| suffix: string, | ||
| html: string, | ||
| ): { unwrappedHtml: string; willUnwrap: boolean } => { |
There was a problem hiding this comment.
This might work better if the params were passed as an object? I'm really just throwing this out there as its a lot subjective (and somewhat IDE dependant) but I always get confused by param ordering and objects solve that.
There was a problem hiding this comment.
After looking deeper, I see this was originally my code 🤦♂
I guess my comment still stands though
There was a problem hiding this comment.
😄 Yeah no worries will update to object, because I actually did trip up with suffix and prefix the wrong way during dev!
| suffix: string, | ||
| html: string, | ||
| ): { unwrappedHtml: string; willUnwrap: boolean } => { | ||
| const willUnwrap = html.startsWith(prefix) && html.endsWith(suffix); |
| @@ -0,0 +1,15 @@ | |||
| const stripTags = (html: string, prefix: string, suffix: string) => { | |||
| return html.slice(prefix.length, html.length - suffix.length); | |||
There was a problem hiding this comment.
I made a mistake here of thinking, hmm, what about substring()...
Several rabbit holes later: ignore me, slice() is good 👍
| }; | ||
|
|
||
| export const BlockquoteComponent: React.FC<Props> = ({ html }: Props) => { | ||
| const blockquote = css` |
There was a problem hiding this comment.
There's a lot of the word 'blockquote' around, maybe blockquoteStyles?
gtrufitt
force-pushed
the
gtrufitt/add-blockquote-element
branch
from
54e3687 to
d9b76b0
Compare
What does this change?
loreminstead oforemBlockquotes are now blockquotes
This PR splits out the
BlockQuoteElementfrom theTextBlockElementand is the prerequisite for guardian/frontend#22215.Note that there is not full styling here, but adding some basic italics to differentiate the quotes.
Before
After
Elements are now top level
It also implements two pieces of
workaroundfor dangerouslySetInnerHtml is not allowed on Fragments.unwrapHtmlbased on aprefixand asuffixto a testable function which takes params:Returning
isUnwrappedif the prefix and suffix match first and last in a string match and returning either modified (matches stripped) html or unmodified html string.RewrappedComponentwhich takes params and generates an HTML element component either based on thetagNameifisUnwrappedistrueandspaniffalse, containing thehtmland addingelCssviaclassName:Before
After
TextComponent storybook uses
loreminstead oforemThis tripped me up... twice 😆 First, I thought I'd caused an issue by removing the first letter, then realised it was because the
Lis omitted in the storybook, but then I realised I actually did have the issue but almost missed it because of the missingL. So this is purely from being able to reason with the story POV...Why?
This allows us to have the proper elements at the top level, so as to:
a) have semantic markup on the page and the correct HTML structure for a11y and SEO reasons.
b) Allow spacefinder to see the correct elements. Note @shtukas - have we worked around this, do we need to adjust spacefinder?
Link to supporting Trello card
https://trello.com/c/kbiUllfA
https://trello.com/c/qL7tNWZT
Note
After all this, I discovered https://github.com/jonathantneal/react-dom-fragment
I don't think the solution we have here is great and it's possible this'll be nicer, so it might be worth reviewing if we touch this code again but I'd be more interested in whether someone creates a proper RFC and implementation in React/Preact.
Anyway.