CPD-11077: Setup versioned fozzie css file. Updated readme, changelog… #105

kevinrodrigues · 2018-01-22T15:25:03Z

I will need to create a PR to update the Fozzie repo so we include a config file which can then be used in the future to overwrite the defaults, I will add this next.

… and unit tests.

… the default boolean value is changed.

ashleynolan · 2018-01-22T16:29:13Z

CHANGELOG.md

+*January 22, 2018*
+
+### Changed
+- Added `useFozziePackageVersion` to config file to handle the css versioning, default value is set to `false`.


Should make these variables more generic – usePackageVersion for example, so that it can be applied to any package that wants to include the package version in the CSS/JS builds.

ashleynolan · 2018-01-22T16:30:53Z

config.js

+         * Fozzie-related variables
+         */
+        fozzieSettings: {
+            useFozziePackageVersion: false,


Also, may be better to make these config settings part of the CSS config object (and the JS config so that they can be set separately).

ashleynolan · 2018-01-22T16:32:04Z

config.js

+        fozzieSettings: {
+            useFozziePackageVersion: false,
+            getFozziePackageVersion: consumingPackageConfig && consumingPackageConfig.version,
+            fozzieBaseName: 'fozzie-'


Not sure you'll need this – I'd just use the name of the SCSS file being generated unless you think there's a good reason for allowing customisation in the settings too?

One of the requirements on the ticket was to version it with the fozzie name e.g. fozzie-[version].css, mostly why I put everything inside a fozzie object :)

Ah ok – the fozzie name should just come from the fozzie repo. So you should be able to just change the name of the base scss file to fozzie.scss and that will be the name of the dist file produced. It's the version part that should get added at the build stage.

Sorry for the confusion!

Ah I see! 👍

ashleynolan · 2018-01-22T16:34:46Z

config.js

+         */
+        fozzieSettings: {
+            useFozziePackageVersion: false,
+            getFozziePackageVersion: consumingPackageConfig && consumingPackageConfig.version,


Again, I'd keep this generic and call it something like packageVersion and put it in the base of the config object. Can see us needing that in other places potentially in future.

Will update this too.

ashleynolan · 2018-01-22T16:37:43Z

tasks/css.js

+        // If the Fozzie package version name is set to `true`, version the css file name with the package number.
+        .pipe(gulpif(config.fozzieSettings.useFozziePackageVersion,
+            rename(cssBasePath => {
+                cssBasePath.basename = config.fozzieSettings.fozzieBaseName + config.fozzieSettings.getFozziePackageVersion;


Would think you'd be able to use something like:

{ suffix: `-${config.fozzieSettings.packageVersion}` }

As similar to when we add the .min below.

ashleynolan · 2018-01-22T16:39:33Z

tasks/css.js

@@ -157,6 +164,13 @@ gulp.task('css:bundle', () => {
            ])
        )

+        // If the Fozzie package version name is set to `true`, version the css file name with the package number.
+        .pipe(gulpif(config.fozzieSettings.useFozziePackageVersion,
+            rename(cssPath => {


I'd add the package number into the rename method below, to the suffix operation. Something like

{ suffix: `-${packageversion}.min` }

DamianMullins · 2018-01-23T09:16:05Z

test/config.test.js

@@ -72,6 +72,50 @@ describe('environment config', () => {

 });

+describe('`fozzieSettings` config', () => {


Could you keep these tests in the same format as the existing tests, please? Makes it much easier to scan through and understand the tests.

Do you mean adding the comments or removing the describe blocks? I will have to change the structure based on the comments anyway.

The other setting use the same description wording ("zzz should be set" & "zzz can be updated") and use the AAA comment separation.

Hmm. I'm not sure following the exact same description here/all the time makes sense? I have a property that shouldn't be changed within this config file so wanted to make that clear in the tests so if it's changed in the config they're fed back with a reason why they shouldn't update it here and not just a failing test. Although the comments make it more readable I think we should avoid them IMHO because they should be self explanatory/readable without them, i.e Assertion is what expect does I would have thought? (Will add them anyway). The reason I didn't follow the AAA and the description is because pathbuilder.tests.js uses a different setup for AAA/describe. Might be a good idea to discuss the way we write tests in the future because I find it harder to understand the current format. For example: I've found it nicer and easier to understand when things are nested to individual properties or methods that belong to the top most scope within describe blocks, because if you have a failing test you can locate it easily (also easier to find where to added further tests), right now if a test fails you have to scan the entire file to find the top level describe (currently not scoped to individual properties) and then scan through the multiple IT statement which is causing the failure. It's also easier to see which scope is broken on the command line.

The main thing I'm concerned with when it comes to the tests is keeping them consistent, easy for newcomers to pick up, and steering clear of complex logic.

Avoiding DRY, sticking to the AAA approach, and asserting against the DOM pretty much solves those issues. As for the describe block, I'm not against using them, I just find that nesting them too much makes the test file harder to follow, and in a recent instance in Global Web, the describe` blocks made it made it really difficult to find the failing tests. I guess it's a case of using them sensibly when it makes sense?

Happy to discuss this as a group, maybe at the next UI Guild meeting?

@kevinrodrigues As someone new to structuring tests (noob alert 🚨 ) best thing to do if you think there is a clearer way to structure the tests would be to make a small (separate) PR example of how you would structure them so we can compare between them (maybe discuss at a UI guild or in a separate meeting once this is done).

As someone with no real affiliation to any structure, happy to then give my opinion 😄

@ashleynolan @DamianMullins I'll put one example together so we can discuss it in one of the UI meetings 👍

kevinrodrigues · 2018-01-25T09:43:54Z

@DamianMullins @ashleynolan @simonsandersje Would you mind having another look at this PR please.

ashleynolan · 2018-01-25T10:11:05Z

config.js

        webRootDir: '.',
        assetSrcDir: 'src',
        assetDistDir: 'dist',
        applyRevision: true,
+        packageVersion: consumingPackageConfig && consumingPackageConfig.version,


More of a question than a comment for change – does this return the value in consumingPackageConfig.version? Looks like it would return a Boolean, but guessing this is a shortcut to test for the config object being present?

It's a shorthand falsy check, it should return undefined if version isn't defined I think.

It's probably OK to just use consumingPackageConfig.version here, we're not doing the falsy check anywhere else in the module.

It will return the version property if it's defined otherwise it returns undefined. Agree, I'll update it to consumingPackageConfig.version.

ashleynolan · 2018-01-25T10:14:31Z

tasks/css.js

@@ -139,6 +139,11 @@ gulp.task('css:bundle', () => {
        .pipe(gulpif(config.isDev,
            sourcemaps.write('.')
        ))
+
+        // If the package version name is set to `true`, version the css file name with the package number.
+        .pipe(gulpif(config.css.usePackageVersion,


If you do the rename here before the sourcemap write above, will the sourcemaps still link properly? (worth checking the name of the file linked in the CSS file produced – may work this out automatically)

Good spot. I checked this and got the below so looks ok to me? I did fix another issue that I spotted while checking this though, the suffix .min wasn't being applied in a falsy scenario (from a change I made) I've added a fix for this now.

ashleynolan

LGTM

DamianMullins · 2018-01-26T09:48:22Z

CHANGELOG.md

@@ -3,6 +3,15 @@
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

+v7.1.0


This should be v7.2.0.

Happy to merge this once updated 👍

Good catch 👍 Updated.

CPD-11077: Setup versioned fozzie css file. Updated readme, changelog…

5f080dc

… and unit tests.

kevinrodrigues requested review from DamianMullins, ashleynolan and simonsandersje January 22, 2018 15:25

kevinrodrigues requested a review from a team as a code owner January 22, 2018 15:25

kevinrodrigues closed this Jan 22, 2018

kevinrodrigues reopened this Jan 22, 2018

CPD-11077: Moved tests into correct scope, fixed tests so it fails if…

c0b7b15

… the default boolean value is changed.

ashleynolan reviewed Jan 22, 2018

View reviewed changes

DamianMullins reviewed Jan 23, 2018

View reviewed changes

kevinrodrigues added 3 commits January 23, 2018 12:53

CPD-11077: Fixed PR comments. Updated tests, changelog and readme.

aeb6b30

CPD-11077: Updated Readme config object.

d87d0a2

CPD-11077: Removed fozzie specific comment.

1d39774

ashleynolan reviewed Jan 25, 2018

View reviewed changes

CPD-11077: Fixed min suffix not being applied to outputted css files.

a543768

kevinrodrigues closed this Jan 25, 2018

kevinrodrigues reopened this Jan 25, 2018

ashleynolan previously approved these changes Jan 25, 2018

View reviewed changes

DamianMullins previously approved these changes Jan 26, 2018

View reviewed changes

DamianMullins reviewed Jan 26, 2018

View reviewed changes

kevinrodrigues dismissed stale reviews from DamianMullins and ashleynolan via aa315a4 January 26, 2018 11:25

CPD-11077: Updated changelog version.

aa315a4

ashleynolan approved these changes Jan 26, 2018

View reviewed changes

kevinrodrigues merged commit bc123d5 into master Jan 26, 2018

kevinrodrigues deleted the CPD-11077-setup-fozzie-base-css-generation branch January 26, 2018 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPD-11077: Setup versioned fozzie css file. Updated readme, changelog… #105

CPD-11077: Setup versioned fozzie css file. Updated readme, changelog… #105

kevinrodrigues commented Jan 22, 2018

ashleynolan Jan 22, 2018 •

edited

Loading

ashleynolan Jan 22, 2018

ashleynolan Jan 22, 2018

kevinrodrigues Jan 22, 2018 •

edited

Loading

ashleynolan Jan 22, 2018 •

edited

Loading

kevinrodrigues Jan 22, 2018

ashleynolan Jan 22, 2018

kevinrodrigues Jan 22, 2018

ashleynolan Jan 22, 2018 •

edited

Loading

ashleynolan Jan 22, 2018

DamianMullins Jan 23, 2018

kevinrodrigues Jan 23, 2018 •

edited

Loading

DamianMullins Jan 23, 2018

kevinrodrigues Jan 23, 2018

DamianMullins Jan 23, 2018

ashleynolan Jan 23, 2018

kevinrodrigues Jan 23, 2018

kevinrodrigues commented Jan 25, 2018

ashleynolan Jan 25, 2018

DamianMullins Jan 25, 2018

DamianMullins Jan 25, 2018

kevinrodrigues Jan 25, 2018

ashleynolan Jan 25, 2018

kevinrodrigues Jan 25, 2018 •

edited

Loading

ashleynolan left a comment

DamianMullins Jan 26, 2018

ashleynolan Jan 26, 2018

kevinrodrigues Jan 26, 2018

		@@ -72,6 +72,50 @@ describe('environment config', () => {

		});

		describe('`fozzieSettings` config', () => {

CPD-11077: Setup versioned fozzie css file. Updated readme, changelog… #105

CPD-11077: Setup versioned fozzie css file. Updated readme, changelog… #105

Conversation

kevinrodrigues commented Jan 22, 2018

ashleynolan Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrodrigues Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

ashleynolan Jan 22, 2018 • edited Loading

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

ashleynolan Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrodrigues Jan 23, 2018 • edited Loading

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

kevinrodrigues commented Jan 25, 2018

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

kevinrodrigues Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

ashleynolan 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

ashleynolan Jan 22, 2018 •

edited

Loading

kevinrodrigues Jan 22, 2018 •

edited

Loading

ashleynolan Jan 22, 2018 •

edited

Loading

ashleynolan Jan 22, 2018 •

edited

Loading

kevinrodrigues Jan 23, 2018 •

edited

Loading

kevinrodrigues Jan 25, 2018 •

edited

Loading