Migrate flat, flat-square, plastic and social to use `XmlElement` #6883

chris48s · 2021-08-11T16:09:06Z

This PR applies the work we started in #5754 to the other 4 styles. This fully delegates the responsibility for XML escaping and formatting/minification to the XmlElement class making it easier to "fall into the pit of success" when it comes to escaping.

One thing to note in review is I've been careful to replicate the existing behaviour such that the outputs exactly match the existing outputs without changing the snapshots at all. This should give us a high level of confidence that these changes don't make any functional changes to the XML output. You can also use http://127.0.0.1:3000/dev/styles/ for a visual side-by-side visual comparison.

I've attempted to rebase the history into something vaguely sensible to try and make the review easier 🤞

this is the only place it is now used

leave everything as XmlElement objects right till the end then make one final call to .render() which cascades aaall the way through the tree

now we've removed all the intermediate calls to render() we can just use an empty string

shields-ci · 2021-08-11T16:09:48Z

	Messages
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!
📖	✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against 870af64

chris48s · 2021-08-11T16:10:53Z

badge-maker/lib/xml.js

+class NullElement {
+  // TODO: we can remove this later
+  render() {
+    return ''
+  }
+}


This was helpful as an intermediate step in the refactoring but it gets deleted in f982039 You can basically ignore it in review

chris48s · 2021-08-11T16:11:17Z

badge-maker/CHANGELOG.md

+## 4.0.0 [WIP]
+
+- Drop compatibility with Node 10


Not strictly related to this PR but the next package release has to be v4 because we've dropped node10. I'm starting the changelog here so we don't lose sight of it.

chris48s · 2021-08-11T16:11:53Z

badge-maker/lib/badge-renderers.js

  }

  static render(params) {
    return new this(params).render()
  }

+  getTextElement({ leftMargin, content, link, color, textWidth, linkWidth }) {


I think there's actually scope to factor out getTextElement() into a shared helper (maybe getBackgroundGroupElement() and getForegroundGroupElement() too), DRY up and have FTB share more logic with flat/flat-square/plastic. At the moment FTB and flat/flat-square/plastic use different attribute order in the XML output though so I think doing this will require tweaking the snapshots. For this reason I think this should be done in a follow-on PR instead of here. The property that we know the code exactly matches the snapshots before and after this PR is probably a valuable property for reviewing plus there is already enough stuff changing at once here.

calebcartwright · 2021-08-17T02:48:32Z

Haven't been able to carve out enough time yet to start going through this in detail but hoping to do so later this week

calebcartwright

Thanks for tackling this one! Everything LGTM. Have left one inline question for my own edification, and one minor (non-blocking) suggested change.

I also want to double check the raster side of the equation out of an abundance of caution (will do here shortly), but we're good to move ahead with this from my perspective

calebcartwright · 2021-08-21T03:03:04Z

badge-maker/lib/badge-renderers.js

+    return this.getTextElement({
+      leftMargin: this.labelMargin,
+      content: this.label,
+      link: !shouldWrapBodyWithLink({ links: this.links }) && leftLink,


Realize this is identical to what was there before, but seems like we should change the ordering here to take advantage of short circuiting, given that links are more the exception than the norm

Suggested change

link: !shouldWrapBodyWithLink({ links: this.links }) && leftLink,

link: leftLink && !shouldWrapBodyWithLink({ links: this.links }),

I accepted this because it looked kinda sensible. Then fortunately a test failed :)
link here isn't a boolean - its the link URL. Order matters
so this is behaving more like link: !shouldWrapBodyWithLink({ links: this.links }) ? leftLink : undefined
Since we both looked at that and thought it was a bool, maybe writing it like that would be clearer?

It looks like this made the tests fail? It's not at all clear to me why that would be the case

oops, comments crossed

Since we both looked at that and thought it was a bool, maybe writing it like that would be clearer?

Yeah that makes sense, it's currently subtle/easy to miss clearly 😄

will go ahead and re-:+1: since this could certainly be deferred like some of other changes mentioned but happy to circle back and do so again if you want to tack that onto this pr

calebcartwright · 2021-08-21T17:14:43Z

badge-maker/lib/xml.js

-    return this.content.reduce((acc, el) => acc + el.render(), '')
+    return this.content.reduce(
+      (acc, el) =>
+        typeof el.render === 'function'
+          ? acc + el.render()
+          : acc + escapeXml(el),
+      ''
+    )


I can't convince myself that I understand this part of the diff 😆 Would you mind explaining it to me?

Yep so basically this.content is a list of things each of which might be:

an XmlElement object

another nested ElementList object

a string (tbh, this is mostly an empty string when we don't want to render a particular element e.g:

shields/badge-maker/lib/badge-renderers.js

Lines 227 to 231 in b6c5a10

const shadow = this.constructor.shadow ? shadowText : ''

if (!link) {

return new ElementList({ content: [shadow, text] })

}

)

so if I say

const list = new ElementList({content: [ new XmlElement({name: 'foo'}), 'bar', new XmlElement({name: 'baz'}), ]})

then I want render() to output

<foo />bar<baz />

this.content.reduce((acc, el) => acc + el, '')

does the same thing as

this.content.join('')

so I'm doing that, but then I'm saying in the reduce function:
If this object has a render() function (it quacks like an XmlElement or an ElementList), call render() and concat the result. Otherwise (assume it is just a string), escape the string and concat that.

tbh, this is mostly an empty string when we don't want to render a particular element e.g:

Ah okay, so even if it's not an element it's not strictly going to be an empty string which requires a more thorough check. makes sense, thanks

Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>

This reverts commit b6c5a10.

chris48s added 10 commits August 11, 2021 16:37

start changelog entry for v4

81a09a9

migrate Flat/FlatSquare/Plastic to use XmlElement

b8822c3

move brightnessThreshold into colorsForBackground

a820225

move old renderLogo function inline into social()

01f24c4

this is the only place it is now used

use XmlElement in social()

fd14c14

don't quote numbers if we don't need to

66383ea

remove intermediate calls to .render()

b577a03

leave everything as XmlElement objects right till the end then make one final call to .render() which cascades aaall the way through the tree

factor out code for assembling logo element

23123fe

use scale consts in social

138c525

remove NullElement

f982039

now we've removed all the intermediate calls to render() we can just use an empty string

chris48s added the npm-package Badge generation and badge templates label Aug 11, 2021

chris48s commented Aug 11, 2021

View reviewed changes

shields-cd temporarily deployed to shields-staging-pr-6883 August 14, 2021 20:38 Inactive

calebcartwright mentioned this pull request Aug 21, 2021

DO NOT MERGE badges/squint#8

Closed

shields-cd temporarily deployed to shields-staging-pr-6883 August 21, 2021 02:46 Inactive

calebcartwright previously approved these changes Aug 21, 2021

View reviewed changes

Update badge-maker/lib/badge-renderers.js

b6c5a10

Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>

chris48s dismissed calebcartwright’s stale review via b6c5a10 August 21, 2021 18:56

chris48s temporarily deployed to shields-staging-pr-6883 August 21, 2021 18:56 Inactive

Revert "Update badge-maker/lib/badge-renderers.js"

05f4f80

This reverts commit b6c5a10.

chris48s temporarily deployed to shields-staging-pr-6883 August 21, 2021 19:07 Inactive

calebcartwright previously approved these changes Aug 21, 2021

View reviewed changes

write leftlink so it doesn't look like a bool

6df7949

chris48s dismissed calebcartwright’s stale review via 6df7949 August 21, 2021 19:26

chris48s temporarily deployed to shields-staging-pr-6883 August 21, 2021 19:27 Inactive

calebcartwright approved these changes Aug 21, 2021

View reviewed changes

Merge branch 'master' into xmlelement2

870af64

chris48s deployed to shields-staging-pr-6883 August 21, 2021 19:37 View deployment

chris48s merged commit e8c78d5 into badges:master Aug 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate flat, flat-square, plastic and social to use `XmlElement` #6883

Migrate flat, flat-square, plastic and social to use `XmlElement` #6883

chris48s commented Aug 11, 2021

shields-ci commented Aug 11, 2021 •

edited

Loading

chris48s Aug 11, 2021 •

edited

Loading

chris48s Aug 11, 2021

chris48s Aug 11, 2021

calebcartwright commented Aug 17, 2021

calebcartwright left a comment

calebcartwright Aug 21, 2021

chris48s Aug 21, 2021

calebcartwright Aug 21, 2021

calebcartwright Aug 21, 2021

calebcartwright Aug 21, 2021

calebcartwright Aug 21, 2021

chris48s Aug 21, 2021

calebcartwright Aug 21, 2021

	link: !shouldWrapBodyWithLink({ links: this.links }) && leftLink,
	link: leftLink && !shouldWrapBodyWithLink({ links: this.links }),

	const shadow = this.constructor.shadow ? shadowText : ''

	if (!link) {
	return new ElementList({ content: [shadow, text] })
	}

Migrate flat, flat-square, plastic and social to use XmlElement #6883

Migrate flat, flat-square, plastic and social to use XmlElement #6883

Conversation

chris48s commented Aug 11, 2021

shields-ci commented Aug 11, 2021 • edited Loading

chris48s Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Aug 17, 2021

calebcartwright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Migrate flat, flat-square, plastic and social to use `XmlElement` #6883

Migrate flat, flat-square, plastic and social to use `XmlElement` #6883

shields-ci commented Aug 11, 2021 •

edited

Loading

chris48s Aug 11, 2021 •

edited

Loading