pattern-lab / patternlab-node

The Node version of Pattern Lab
http://patternlab.io
MIT License
2.05k stars 406 forks source link

[uikit-workshop] View All for sub types is broken #890

Closed engelfrost closed 4 years ago

engelfrost commented 6 years ago

I am using Pattern Lab Node v3.0.0-alpha.16 on Mac, with Node v9.6.1, using a Vanilla Edition.

image

This sound similar to #835.

Expected Behavior

Load the view localhost:3000/?p=viewall-templates-onboarding (which works if I type the URL)

image

Actual Behavior

image

Steps to Reproduce

Try to open a View all page for a sub type.

Gumbtastic commented 6 years ago

I'm not sure, if this is already known, but this bug only occours, when the patterns are orderd by double digit prefix like so: "00-global". Having the patterns orderd alphabetically causes no problems.

jeffwhitfield commented 6 years ago

This is still an issue with v3.0 beta 1. Just installed a fresh copy using the Mustache demo. "View All" links just don't work. For instance, going to Organisms->Global->View All results in the behavior described above. As mentioned above, doing away with ordered groups seems to work...but considering most people want to order their patterns, this definitely needs to be addressed.

latviancoder commented 5 years ago

Experiencing the same problem with v3.0.0-beta.1.

BlushingFox commented 5 years ago

I'm not sure, if this is already known, but this bug only occours, when the patterns are orderd by double digit prefix like so: "00-global". Having the patterns orderd alphabetically causes no problems.

Just installed v3.0.0-beta.1 and having the same issue.

sghoweri commented 5 years ago

I've been digging into this bug for a good couple hours yesterday and today -- still no smoking gun but I definitely feel like I'm getting close to finding one.

Here's a quick summary of things I've been able to narrow this issue down to:

  1. It's probably not being caused by async loaded JavaScript / dynamic imports.

The fact that this issue occurs even when the Pattern Lab JS is not loading asynchronously (ie render blocking script tags + no dynamic imports) can almost entirely rule out this being a load order issue.

image

Whether or not it's an issue with the order things get run (vs just loaded) remains to be seen.

  1. I also found that the issue still occurs when all non-essential JavaScript is removed from patternlab-pattern.js and patternlab-viewer.js which at least narrows down the number of places to keep digging if the fix for this will require a front-end JS update.. not sure about that yet though.
// patternlab-viewer.js
import '@webcomponents/custom-elements/src/native-shim.js';
import './components/pl-layout/pl-layout';
import './components/styleguide'; // --> not yet refactored... hmmmm 

// patternlab-pattern.js
import './utils/postmessage'; // --> what updates the page when nav links are clicked
  1. One clue I've seen is that the bug here never seems to happen when there aren't any top-level pattern-specific markdown files.

For example, after removing the two top-level markdown files in the handlebars dev edition -- https://github.com/pattern-lab/patternlab-node/blob/dev/packages/development-edition-engine-handlebars/source/_patterns/atoms/type.md and https://github.com/pattern-lab/patternlab-node/blob/dev/packages/development-edition-engine-handlebars/source/_patterns/molecules/variants.md -- the viewall issue here seems to disappear...

image

  1. One other potential clue I've noticed when the problem happens for me: the patternPath URLs generated seem to be different -- the viewall pages that seem to be broken (or at least the ones that were broken for me in handlebars dev edition) were the ones referencing paths ending in ".rendered.html"... 🤔

image

Working?

image

Broken

CC @bmuenzenmeyer

bmuenzenmeyer commented 5 years ago

@sghoweri I've narrowed down the difference in each approach to ui_builder.js injectDocumentationBlock(). The broken subtypes already have a docPattern found (due to their having markdown I suspect).

I can monkey-patch the correct patternLink a number of different ways here, but the main problem to me seems like loadPattern assumes this property change will update the "computed" property patternLink

So, I want to fix that. But even monkey-patching this quick still gives me this error:

image

Likely there is an additional bug underneath this one. My next steps will be to better understand the state of patterns within injectDocumentationBlock(), since they are returned as found (broken upstream) or created just-in-time (working).

Stay tuned

james-nash commented 5 years ago

We've recently migrated from PL2 to PL3 and found the same issue. Glad to see it's already being worked on.

In case it's of any use, we have a hosted copy of our pattern library here: http://style-staging.buildit.digital/ where you can easily reproduce the issue - for example, try "Molecules" > "Lists" > "View all". The corresponding source code can be found here: https://github.com/buildit/gravity-ui-sass/tree/develop/src/styleguide

FWIW, most of our patterns have markdown files so perhaps that adds some weight to your suspicions (and those raised in #970) that this triggers the bug somehow.

Hope that helps!

bmuenzenmeyer commented 5 years ago

Hoping to devote an hour or so to this soon!

sghoweri commented 5 years ago

@bmuenzenmeyer I think I might have a fix for this -- PR should be up shortly!

willthemoor commented 5 years ago

Just having an initial play with 3.0 and ran into this issue. I'm using edition-twig and starterkit-twig-demo without any customizations or changes from what npm installs.

Not sure if this report is helpful at this point but sharing just in case.

tl;dr: markdown files didn't seem to matter. Numeric folder naming did.

After reading through issues I tried dumping all markdown files by running this from the _patterns folder.

find . -name '*.md' -print0 | xargs -0 rm

but the View All issue persisted.

Then, I went into the 03-organisms folder and removed all numerical prefixes from the folder names.

/00-global/   --> /global/
/01-article/  --> /article/
/02-comments/ --> /comments/
/03-sections/ --> /sections/

Restarted the PL server and View All worked as expected.

bmuenzenmeyer commented 5 years ago

@sghoweri added this to a new milestone - can you add any PRs or pressing business in priority order too?

JosefBredereck commented 4 years ago

Fixed with #1143 in V5.12.0