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

Feature/next/fractal #262

Closed
wants to merge 2 commits into from
Closed

Feature/next/fractal #262

wants to merge 2 commits into from

Conversation

c1rrus
Copy link
Contributor

@c1rrus c1rrus commented May 16, 2019

Closes #264
Related to #106

Description
Ditches Pattern Lab and migrates to Fractal as our tool for generating the pattern library website.

While Pattern Lab has served us well over the years, there are additional features that Fractal has which are useful to us:

  • Ability to use different preview layouts per component. E.g.: We could use this to have a preview that overrides the usual flex on body stuff for individual components, and another which does not for templates and pages
  • Grouping component files into folders. It's nice to have the data, docs and template altogether as a unit. This also makes it easier if we want to reorder or reorganise components, since we only need to change the directory name
  • We can use Fractals API to make a customised component template export function (they have an example and I have done something very similar in a personal project, so can confirm it works). This will help pave the way to publishing the Nunjucks templates as an NPM package for other projects to consume
  • Ability to use JS files to provide context data. We could use this to auto-generate dummy content or fetch realistic content from some APIs. Saves writing everything out as JSON manually!

Notes for reviewers
The vast majority of changes are simple file renames. Where individual patterns in Pattern Lab were like this:

some/
    category/
        12-pattern-name.njk   # template
        12-pattern-name.md    # description text
        12-pattern-name.json  # context data

The equivalent files in Fractal are like this:

some/
    category/
        12-pattern-name/
            pattern-name.njk          # template
            README.md                 # description text
            pattern-name.config.json  # context data

Additionally, the JSON files need a top-level context property, whose value is the context data. This is because these files can contain other kinds of per-pattern configuration info too.

The more interesting changes are in the Gulp scripts. All Pattern Lab stuff has been removed and replaced with the equivalent Fractal functionality. Feel free to ping me if you want me to talk you through any of it.

@c1rrus c1rrus added WIP STATUS: Work in Progress - Do Not Merge. Useful for long-running PRs infrastructure CATEGORY: Infrastructure related - e.g. updates to build process, tests, tooling, etc. labels May 16, 2019
@c1rrus c1rrus added this to the Gravity NEXT milestone May 16, 2019
@c1rrus c1rrus force-pushed the feature/next/fractal branch from 23a536b to 3f645bd Compare May 19, 2019 17:19
Copy link
Contributor

@ashblue ashblue left a comment

Choose a reason for hiding this comment

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

Minor adjustments. Please be sure to include a linter rule for whitespace.

// match: /<\/head>/i,
// },
// },
// });
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover comment block, delete or restore please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot. It's been restored now.

}
buildPatternLibrary.displayName = `${taskNamePrefix}build`;


Copy link
Contributor

Choose a reason for hiding this comment

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

Please include a lint check for concurrent whitespace.

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've just rebased on the current next branch, which contains your recent, shared ESLint setup. I've fixed all the stuff that was being flagged - including the double blank lines - and it's oh so pretty now! ^_^

@c1rrus c1rrus force-pushed the feature/next/fractal branch from 3f645bd to 45424a7 Compare May 23, 2019 21:07
@james-nash james-nash force-pushed the feature/next/fractal branch 2 times, most recently from 831e17f to 4d0c524 Compare May 24, 2019 16:05
@c1rrus c1rrus force-pushed the feature/next/fractal branch from 4d0c524 to 59e9e19 Compare May 24, 2019 23:01
@c1rrus c1rrus force-pushed the feature/next/fractal branch from 2dd46ed to b08a218 Compare June 4, 2019 21:51
@james-nash james-nash force-pushed the feature/next/fractal branch from b08a218 to df1bf4d Compare June 5, 2019 15:59
@james-nash james-nash force-pushed the feature/next/fractal branch from df1bf4d to db417cc Compare June 19, 2019 13:33
@james-nash james-nash force-pushed the feature/next/fractal branch from db417cc to e3a3b70 Compare August 16, 2019 15:25
affects: @buildit/gravity-ui-nunjucks
@james-nash james-nash marked this pull request as ready for review August 16, 2019 16:02
@james-nash james-nash removed the WIP STATUS: Work in Progress - Do Not Merge. Useful for long-running PRs label Aug 16, 2019
Copy link
Collaborator

@danbull danbull left a 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! I think this is probably an example of a feature that may have to be manually merged in? #329

@james-nash james-nash removed the request for review from ashblue August 20, 2019 12:48
@james-nash
Copy link

I think this is probably an example of a feature that may have to be manually merged in? #329

Not sure what you mean? I just had a look and don't think anything related to #329 got lost in this transition.

@danbull
Copy link
Collaborator

danbull commented Aug 21, 2019

I think this is probably an example of a feature that may have to be manually merged in? #329

Not sure what you mean? I just had a look and don't think anything related to #329 got lost in this transition.

Apologies, long story but ignore me, can confirm it's all good.

@james-nash james-nash removed the request for review from dw-buildit August 22, 2019 10:16
@james-nash james-nash closed this Aug 22, 2019
james-nash pushed a commit that referenced this pull request Aug 23, 2019
@james-nash
Copy link

FYI: This did get merged. Github somehow got a bit confused though, so I had to "close" this PR afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure CATEGORY: Infrastructure related - e.g. updates to build process, tests, tooling, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants