improvement: ability to configure default pattern info code panel #1155

[mfranzke]

Closes #1155

Summary of changes:
Added the configuration option for the ability to configure default pattern info code panel.

[bmuenzenmeyer]

Thanks for this! It's also SUPER EXCITING to me to see docs and functionality changing in lockstep finally. It's the realization of a goal we've had for a long time.

I've grown accustomed to setting the default value of any new config value in patternlab-config.json to improve discovery. (In the new core, I may revisit this pattern and create a more intentional extensibility model where users need only supply values they wish to override.... it sort of works like this already in fact!)

but for now, can you add this key and its default to:

Most Important

• packages/core/patternlab-config.json (used within getDefaultConfig)
• packages/edition-node/patternlab-config.json
• packages/edition-node-gulp/patternlab-config.json
• packages/edition-twig/patternlab-config.json

Less Important or Internal

• packages/starterkit-twig-demo/patternlab-config.json (merged in namespaces)
• packages/core/test/util/patternlab-config.json
• packages/cli/test/fixtures/patternlab-config.json
• packages/development-edition-engine-handlebars/patternlab-config.json
• packages/development-edition-engine-react/patternlab-config.json
• packages/development-edition-engine-twig/patternlab-config.json

[mfranzke]

I've grown accustomed to setting the default value of any new config value in patternlab-config.json to improve discovery. (In the new core, I may revisit this pattern and create a more intentional extensibility model where users need only supply values they wish to override.... it sort of works like this already in fact!)

but for now, can you add this key and its default to:

I was struggling with the aspect of how to reflect (and that for integrate) an optional config in the first place (as an opposition to at least most of the other configuration values to my impression), and your explanation makes perfectly sense to me.

[mfranzke]

@bmuenzenmeyer, are you 100% (as in totally, or without a doubt 😉) sure that it even also needs to get added to packages/starterkit-twig-demo/patternlab-config.json? I did so, but due to your comment and the files content, it feels "wrong".

As you're going full monty on all of the different variations of patternlab (which I totally agree with), I'd like to additionally add that I've only tested node/handlebars myself manually (as this is the one I'm most experienced with). If there aren't any test (cases) to reflect/check this new behavior for this and the other editions, I could even also offer to try to do these manual regression tests on those.

[bmuenzenmeyer]

You can omit it from that one, it's an outlier.

[mfranzke]

You can omit it from that one, it's an outlier.

Okay, I've reverted that one. I think we're good to go now.

[bmuenzenmeyer]

I was struggling with the aspect of how to reflect (and that for integrate) an optional config in the first place (as an opposition to at least most of the other configuration values to my impression), and your explanation makes perfectly sense to me.

I owe you an apology @mfranzke - I've been struggling with how to tell you I hate my own advice - so here we go! I don't think it appropriate to add in mustache to these configs, especially when that is increasingly not an option folks choose. I'd rather we:

• keep this an option only documented, and omit it from configs
• make a special key that is inherit or something that makes it clearer it uses the patternExtension value.

I prefer the first, if I had to choose.

I really like that you fallback to patternExtension - that's probably enough for most people to use this as a smart default.

[mfranzke]

Easy peasy, I even also do like to think things over again and especially if it’s for the better afterwards, it‘s more important to be open for those changes than to follow a worse route just because.

[mfranzke]

@bmuenzenmeyer, I reverted commit 53d2dd6 altogether. I interpreted your previous comment in the way that the config option shouldn't be included within all config files (not only the mustache ones), but only documented. If you only did mean the mustache related configs to get reverted (meaning, that only they shouldn't include the mustache file extension as a default as well), please come back to me.

Elsewhere we should be good to go now.

[bmuenzenmeyer]

I'm not sure what's going on yet but I cannot get this to display the code panels anymore

https://github.com/pattern-lab/patternlab-node/tree/mfranzke-improvement-ability-to-configure-default-pattern-info-code-panel-test

packages/development-edition-engine-handlebars
yarn pl:serve

[mfranzke]

I'm not sure what's going on yet but I cannot get this to display the code panels anymore

https://github.com/pattern-lab/patternlab-node/tree/mfranzke-improvement-ability-to-configure-default-pattern-info-code-panel-test

packages/development-edition-engine-handlebars
yarn pl:serve

@bmuenzenmeyer I'm not totally sure, whether I do understand your current test correctly.

If your "serving" the system from the files out of https://github.com/pattern-lab/patternlab-node/tree/mfranzke-improvement-ability-to-configure-default-pattern-info-code-panel-test/packages/development-edition-engine-handlebars than the changes out of this PR #1156 wouldn't have any effect, because the dependency you're retrieving is still the actual UIKit w/o the change that I've provided with this PR #1156.
The "reason why" that within this branch none of the tabs is being hightlighted / active even though that you've even already defined an active tab via the new configuration (in this case "html") is that this specific value "html" is only going to be correctly adapted with the code that I'm providing with this very own PR #1156 itself (but which is not included in serving from this folder at the moment, as it's not published).

Or to put it in other words, it really depends on your expected testing results:

• Highlighting the first tab (e.g. "hbs") would mean to either totally leave out the configuration option defaultPatternInfoPanelCode, or to define it as "defaultPatternInfoPanelCode": "hbs" (even though that this wouldn't test the changes out of this PR, as this is the default)
• Even also highlighting the "scss" tab has been enabled with my previous PR, but than you would need to define "defaultPatternInfoPanelCode": "scss", (even though that this wouldn't test the changes out of this PR, as this has been introduced/enabled with the "previous" PR Fix plugin-tabs enabling multiple file formats #1163 #1166)
• Expecting the html tab to be initially active by defining "defaultPatternInfoPanelCode": "html", wouldn't work at the moment, as my change out of this PR improvement: ability to configure default pattern info code panel #1155 #1156 hasn't been released and the code you're building up on to still is based on @pattern-lab/uikit-workshop / version 5.9.3 without my code changes included.
  So to test this last aspect, you would need to build the contents out of https://github.com/pattern-lab/patternlab-node/tree/mfranzke-improvement-ability-to-configure-default-pattern-info-code-panel-test/packages/uikit-workshop/, installing the resulting uikit-workshop package files and than serving the files from https://github.com/pattern-lab/patternlab-node/tree/mfranzke-improvement-ability-to-configure-default-pattern-info-code-panel-test/packages/development-edition-engine-handlebars.

Sorry for the long explanation - as I'm not totally sure whether I'm getting your point correctly (or at all) I just wanted to write down my thoughts in general, and I do hope that it makes sense.

[bmuenzenmeyer]

it's alright - I should have been more clear. The branch I linked above pulled in your fork and branch local to this repo, so the changes should be present. This is the common fork testing pattern that GH suggests:

I went a step further though, by adding patterns into the development edition and switching to html as you mention. this should be using the local uikit build - but something seems off. It is very possible I am testing incorrectly.

[mfranzke]

it's alright - I should have been more clear. The branch I linked above pulled in your fork and branch local to this repo, so the changes should be present. This is the common fork testing pattern that GH suggests:

I went a step further though, by adding patterns into the development edition and switching to html as you mention. this should be using the local uikit build - but something seems off. It is very possible I am testing incorrectly.

@bmuenzenmeyer okay, the testsetup makes perfectly sense and is aligned with mine.
Could you please provide a screenshot ? When I'm trying those testing steps, I get the following UI for the pattern details - it's mainly the tabs not being active at all:

And / or are there any JS errors on the dev tools console ?

My assumption that the code that I've provided is missing within the browser, is possible to check by opening the file /styleguide/js/pl-modal-viewer-chunk-*.modern.js (replace * by whatever hash is being generated in your build) and searching e.g. for pl-panel-html - the related key default still has the value !1 that doesn`t reflect my changes out of

patternlab-node/packages/uikit-workshop/src/scripts/components/panels.js

Line 86 in de2b492

default: window.config.defaultPatternInfoPanelCode && window.config.defaultPatternInfoPanelCode === 'html',

[mfranzke]

@bmuenzenmeyer, I’ve rechecked this whole topic, and the following simple commands now work perfectly fine for me for retesting these changes - from the root of your branch (https://github.com/pattern-lab/patternlab-node/tree/mfranzke-improvement-ability-to-configure-default-pattern-info-code-panel-test) just run the following commands (as aligned with the description on https://github.com/pattern-lab/patternlab-node/tree/dev/packages/uikit-workshop#working-on-pattern-labs-ui-locally; probably you were missing the setup initially, at least I did and came up with comments like the previous one):

yarn run setup &&
cd packages/development-edition-engine-handlebars/ &&
yarn dev

This brings up the PL UI with an active HTML tab at the very beginning:

[mfranzke]

Additionally there aren't any problems appearing on the CI/CD netlify test deploy: https://deploy-preview-1156--patternlab-handlebars-preview.netlify.app/

Please keep in mind that it doesn't display the "html" tab as the default as the configuration itself hasn't been part of this merge request. But the netlify preview at least shows, that there aren't any errors being displayed due to these changes.

This should be safe to get tested again and get merged afterwards.

[mfranzke]

my impression is that @sghoweri was a little bit more in detail involved in developing (and testing) UI Kits. @sghoweri, do you have a chance in testing and validating the change out of this ticket? (see my comments from 23 May and 2 July)

Review is outdated