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

Ensure that patterns in direcorties greater than 2 levels deep from s… #1016

Merged

Conversation

david-moldenhauer
Copy link
Contributor

…ource are sorted to the next possible parent directory.

This allowes file structures like:
00-atoms/03-controls/00-button

  • button.pattern
  • button.md
  • button.scss
  • button.js

Closes #

Summary of changes:

…ource are sorted to the next possible parent directory.

This allowes file structures like:
00-atoms/03-controls/00-button
 - button.pattern
 - button.md
 - button.scss
 - button.js
….length·>·1)` with `·((this.subdir.match(/\w(?=\\)|\w(?=\/)/g)·||·[]).length·>·1)·

fixed error Replace `↹` with `··`
@coreylafferty
Copy link

Is it an easy change to make this so it will nest components with the same name as their folder under the same parent menu item in the PL menu? For example, I've tried this folder structure:

/03-components/grid/grid.twig
/03-components/grid/grid--2col.twig
/03 components-grid/grid--3col.twig

What comes out in the PL menu is:

  • components
    -- grid (folder)
    ---- grid 2 column (component)
    ---- grid 3 column (component)
    -- grid (component)

Ideally it would be:

  • components
    -- grid (folder)
    ---- grid (component)
    ---- grid 2 column (component)
    ---- grid 3 column (component)

Currently I've just named the folder 'grids' and then it properly nests them, so I'm assuming it's a conflict with the filename and folder name being the same.

@david-moldenhauer
Copy link
Contributor Author

@coreylafferty
Patternlab only supports 2 levels under the pattern-source directory. What this minor change does is remove all deeper folder information and save the reference in the second level parent folder.
The pattern name is still generated by the pattern file name.
so 'first-level/second-level/third-level/patternfile.ext' becomes 'first-level/second-level' + 'patternfile'
meaning the third-level folder will be ignored as would a fourth-level and so forth.

@coreylafferty
Copy link

coreylafferty commented May 23, 2019

@david-moldenhauer Apologies -- I should have looked at this closer. At first glance I thought this was further improvements to how menu items are nested from the first or second level folders after the initial change to support two levels from PR #992

@sghoweri
Copy link
Contributor

@david-moldenhauer Apologies -- I should have looked at this closer. At first glance I thought this was further improvements to how menu items are nested from the first or second level folders after the initial change to support two levels from PR #992

FWIW, I'd really like to see us add support for deeper levels of nested navigation!

@david-moldenhauer
Copy link
Contributor Author

BTW. The tests fail because they are set up with more than 2 levels deep file structure (as is currently not supported). Is this a theoretical thing in case an extra level does get support at some time? As they are now the tests would fail but run on the second navigation level in the browser or succeed and not run in the browser at all.

Why Pattern.info.hasDir exists and why Pattern.getDirLevel removes the last folder when this is true does not make sense to me. I'm guessing this is for a currently not functioning workaround to support a third sublevel if the folder and pattern name match.
@stale
Copy link

stale bot commented Jul 24, 2019

It's hard to keep track of everything. This issue has been automatically marked as stale because it has not had recent activity, neither from the team nor the community. It will be closed if no further activity occurs. Please consider adding additional info, volunteering to contribute a fix for this issue, or making a further case that this is important to you, the team, and the project as a whole. Thanks!

@bmuenzenmeyer bmuenzenmeyer added the pinned 📌 Don't let stalebot clean this up label Aug 23, 2019
@bmuenzenmeyer
Copy link
Member

Is this a theoretical thing in case an extra level does get support at some time? As they are now the tests would fail but run on the second navigation level in the browser or succeed and not run in the browser at all.

This and your commit comment show me the stretch marks and crow's feet of open source over time. I am not familiar with how that particular code evolved during my time away, but I am assuming it was #992

I plan to review this code in earnest soon

@bmuenzenmeyer
Copy link
Member

Ugh I merged this wrong

@bmuenzenmeyer bmuenzenmeyer merged commit 9d725c5 into pattern-lab:dev Feb 3, 2020
@JosefBredereck
Copy link
Contributor

JosefBredereck commented Apr 10, 2020

As I see now this pull was not thought of correctly. The Idea behind that was never to have deeper nested folders. The original idea was completly ignored and removed. Even the test where changed that they pass the given behavior, not the wished one. I just updated the version an now patterns are not loaded correctly. I will try to fix this error and also take the aproach in mind that deeper nested folders that are not patterns will be moved to a higher level route.

Please see the following discussion and pull request for more information
Added separated pattern-folder support

@bmuenzenmeyer
Copy link
Member

hi @JosefBredereck

I am sorry that I've mismanaged this feature - I came into it late and tried my best to play catch up - but clearly fell short. I need people like you with consistent, long-lasting interest and ability to help keep the codebase in check!

@JosefBredereck
Copy link
Contributor

JosefBredereck commented Apr 11, 2020

@bmuenzenmeyer no problem. I was abstinent by myself and just tipped over that right now. I also saw an issue related to that change.

To get this change correctly. The intention was to move sub-dirs to the nearest top level if you accidentally added it or want to structure your files. At this point, we have to differentiate between pattern definition as directory and just a folder with patterns for organizational purposes.

- molecules/
  - blocks/
    - media-block/
      - media-block.md
      - media-block.mustache

    - some-random-folder
      - random-comp1.json
      - random-comp1.md
      - random-comp1.mustache
      - random-comp2
        - random-comp2.json
        - random-comp2.md
        - random-comp2.mustache
      - another-random-folder
        - random-comp3.json
        - random-comp3.md
        - random-comp3.mustache
        - random-comp4
          - random-comp4.json
          - random-comp4.md
          - random-comp4.mustache
        ...

    - button/
      - button.md
      - button.mustache
      - button~primary.json
    - jumbotron.md
    - jumbotron.mustache
  - heading/
    - heading.md
    - heading.mustache
  - link.mustache
  - link.md
- card/
  - card.md
  - card.mustache
...

Given this tree, it should result that some-random-folder and another-random-folder would be removed and the components random-comp1.mustache, random-comp2.mustache, random-comp3.mustache and random-comp4.mustache will be attached to blocks as long as deeper nesting is not supported. This can go deep endlessly. Also be aware that deeper nesting could contain a nested pattern.

<ol>
  <li>Molecules
    <ol>
      <li>Blocks
        <ol>
          <li>Media Block</li>
          <li>Button</li>
          <li>Button Primary</li>
          <li>Jumbotron</li>
          <li>random-comp1</li>
          <li>random-comp2</li>
          <li>random-comp3</li>
          <li>random-comp4</li>
        </ol>
      </li>
      <li>Heading</li>
      <li>Link</li>
    </ol>
  </li>
  <li>Card</li>
</ol>

While this is the case you always have to keep in mind that the factory can still find the associated .md and .json files.

I'm working on implementing this behavior right now and hopefully will add a pull request this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned 📌 Don't let stalebot clean this up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants