-
Notifications
You must be signed in to change notification settings - Fork 327
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
Generate fixtures.json files for components on build:package #1925
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good – great to have tests that cover this as well.
tasks/gulp/copy-to-destination.js
Outdated
fixtures.examples.push(fixture) | ||
}) | ||
|
||
fs.writeFileSync(fixturesPath, JSON.stringify(fixtures, null, 2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit strange to write the file out from the task, rather than passing it out and letting gulp handle it. It's definitely strange that we write the two files in two different ways.
I think there's probably a more 'gulp-like' way of doing this, but my gulp knowledge is lacking…
If not, and we stick with using fs.writeFileSync
here I think we should write both files from within the task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separately, we use 4 space indentation for params.json
, so we should probably be consistent and use the same here.
dd1d173
to
940d413
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good to me 👍 Agree with @36degrees that it would be good to generate the two files in a similar manner.
940d413
to
a7ec796
Compare
a7ec796
to
08e375d
Compare
dbeea0e
to
ce6a18c
Compare
ce6a18c
to
f50d279
Compare
@hannalaakso @36degrees Thanks both - I think I've addressed those comments now. I've also added a second commit to create separate functions for the fixtures and macro-options gulp tasks - I think it makes it a bit easier to quickly scan the gulp task and see what's happening, but happy to leave that change out if people don't find it helpful! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, with a couple of minor comments 👍🏻
I think it might be revisiting this task some point soon as it feels like it's becoming a bit of a 'monolith' task with too many responsibilities – the two JSON-generating functions could be separate dependent tasks?
That would also make it possible to update just the JSON files without having to run all of the other changes, which feels like it might be useful.
f50d279
to
d958514
Compare
d2234cd
to
0bd7652
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
d958514
to
efd91f7
Compare
Needs rebasing against master once #1924 has been merged |
5f8a662
to
576d450
Compare
Fixtures files contain the component name and all examples with the output html to allow maintainers to test their own HTML against ours
576d450
to
69c8a89
Compare
69c8a89
to
39a638a
Compare
Resolves #1895
What
Adds steps to the
build:package
task to generate fixtures.json files for each component in the following format:Fixtures files contain the component name and all examples with the output html to allow maintainers to test their own HTML against ours