Gather additional feedback when testing the SDC conversion script

Kristen Pol → credited jacobadeutsch → .

See @jacobadeutsch's feedback from here:

https://www.drupal.org/project/demo_design_system/issues/3465043#comment... 📌 Create automated script for converting components to SDC Fixed

In the button component, the yml file was generated with a null default for this var:

    type:
      type: string
      title: Type
      description: 'Button type: primary, secondary, tertiary.'
      default: null
      enum:
      - primary
      - secondary
      - tertiary

I think it's getting that from here:
{% set type = type in ['primary', 'secondary', 'tertiary'] ? type : null %}

Should it just not have a default in this instance? Maybe default: null is right here, I'm not really sure.

Another thing, booleans that appear under objects don't go by the title: description formatting. Here's an example from the list organism:

...
    link_above:
      type: object
      title: Link above
      description: 'Link object:'

      properties:
        text:
          type: string
          title: Text
          description: Text.

        url:
          type: string
          title: Url
          description: URL.

        is_new_window:
          type: boolean
          title: Is new window
          description: Open link in a new window.
...

Repurposing this issue to just gather feedback as we may not need to update the script. It will depend on the issues found.

Some notes when running the new version:

1. Component name could be capitalized since it will be shown in the UI
2. Enum list items should be indented two spaces based on examples I've seen
3. Not sure we need the libraryOverrides as it will pick up the js files automatically

I think any instances of duplicate enums would be because the twig files have a mistake in them. So I wouldn't think it's 100% necessary for the script to do that. Seeing that mistake in the yml helps us find the problem in the twig as well.

Agree +1

When running yamllint, it picks up the indentation errors:

kristens-mbp-2:button kristenpol$ yamllint button.component.yml 
button.component.yml
  1:1       warning  missing document start "---"  (document-start)
  12:3      error    wrong indentation: expected 4 but found 2  (indentation)
  22:7      error    wrong indentation: expected 8 but found 6  (indentation)
  31:7      error    wrong indentation: expected 8 but found 6  (indentation)
  42:7      error    wrong indentation: expected 8 but found 6  (indentation)
  52:7      error    wrong indentation: expected 8 but found 6  (indentation)
  67:7      error    wrong indentation: expected 8 but found 6  (indentation)

Markdown linting:

kristens-mbp-2:button kristenpol$ markdownlint README.md
README.md:8:81 MD013/line-length Line length [Expected: 80; Actual: 156]

It would be nice to have an empty line before "slots":

  type: object
  properties: {}
slots:
  
  column_one:
    title: Column one
    description: First column slot.
~                                     

I found a variable in the campaign component yml which isn't directly taken from the twig:

          type:
            type: string
            title: Type
            description: 'Button type: primary, secondary, tertiary.'
            default: null
            enum:
            - primary
            - secondary
            - tertiary

the twig comment:

{#
/**
 * @file
 * Campaign component.
 *
 * Variables:
 * - content_top: [string] Content slot.
 * - image: [object] Image object:
 *   - url: [string] Image URL.
 *   - alt: [string] Image alt text.
 * - image_position: [string] Image position: left, right.
 * - tags: [array] Array of tags.
 * - title: [string] Title.
 * - date: [string] Date.
 * - content: [string] Content.
 * - links: [array] Array of link objects containing:
 *   - text: [string] Link text.
 *   - url: [string] Link URL.
 *   - is_new_window: [boolean] Open link in the new window.
 *   - is_external: [boolean] Link is external.
 * - content_bottom: [string] Content slot.
 * - theme: [string] Theme: light, dark.
 * - vertical_spacing: [string] With top, bottom or both vertical spaces.
 * - attributes: [string] Additional attributes.
 * - modifier_class: [string] Additional classes.
 */
#}

This I think is because this type var is being taken from the button component which is included here in the twig:

										{% include '@atoms/button/button.twig' with {
											theme: theme,
											kind: 'link',
											type: loop.index == 1 ? 'primary' : 'tertiary',
											text: link.text,
											url: link.url,
											is_new_window: link.is_new_window,
											is_external: link.is_external
										} only %}

To be honest I have no idea if this is supposed to be how it is, so just double checking here.

I would think the variables from includes/embeds should just be defined in that component but I'm not 100% sure.

Working on the feedback.

In the button component, the yml file was generated with a null default for this var:

  type:
      type: string
      title: Type
      description: 'Button type: primary, secondary, tertiary.'
      default: null
      enum:
      - primary
      - secondary
      - tertiary

I think it's getting that from here:
{% set type = type in ['primary', 'secondary', 'tertiary'] ? type : null %}

Should it just not have a default in this instance? Maybe default: null is right here, I'm not really sure.

@jacobadeutsch You're right, it's picking up from the set type snippet you mentioned. The script checks the variable set statements and determines if there's any default or in case of condition any value is being set in else block.

As per the above set type statement default: null is right but let me know in case of null do we want to remove default key.

Since it's from the code, let's keep that one and we can fix manually later if it's a problem.

1. Remove default values from enum. - Fixed
2. Remove libraryOverrides - Fixed
3. Capitalize component name - Fixed
4. Fix issues related to yamllint - Fixed
5. Add empty line before slot when there are no props properties - Fixed

Pending
The point regarding the include variables is still pending, as we are waiting for the final decision on it.

Amazing 🤩 I’ll try to test today 🙏

Fyi, been packing up and prepping for Drupal GovCon, so haven't had a chance to test yet but have assigned a couple issues for community testing and Jacob will be able to test tomorrow.

Jacob has time today… I’m on a plane ✈️

Most of the boolean problems seemed fixed but I did come across a weird edge case:

mobile-navigation-menu.component.yml

name: Mobile navigation menu

status: experimental

group: Organisms

props:

  type: object

  properties:
    theme:
      type: string
      title: Theme
      description: 'Theme: light, dark.'
      default: light
      enum:
        - light
        - dark

    items:
      type: array
      title: Items
      description: 'Menu links array. Each item contains:'
      items:
        type: object

        properties:
          title:
            type: string
            title: Title
            description: The title of the link.

          url:
            type: string
            title: Url
            description: The link url.

          below:
            type: array
            title: Below
            description: Array of submenu items.

          is_expanded:
            type: boolean
            title: Is expanded
            description: Flag if the current item has visible child.

          in_active_trail:
            type: boolean
            title: In active trail
            description: Flag if the current item is in the active trail.

I wish I knew more about yml to understand how this file is working better... The items are being nested twice or something? Coding this all seems like a pain, thank you for doing such a great job

Will get this fixed today.. must have missed some case.. thanks for testing.

Here's a list of all of current yamllint and markdownlint errors and their associated files. I recommend turning this back into a json so it looks nicer, it wouldn't let me upload json. This is from a script I've almost finished the first iteration of, I could have it out to use by tomorrow if I'm able to meet with Kristen enough by then. Not sure though because she's at a conference right now.

Thanks! We can ignore the:

warning  missing document start \"---\"  (document-start)":

warnings but the 80 character ones should be addressed if possible (whether manually or in the script).

I went through and fixed most of the remaining stuff manually. The READMEs all had this error: 8:81 MD013/line-length Line length [Expected: 80; Actual: 156] and I just split that line into two. So that's one thing the generator script could have a small update on. Other than that I also fixed some line length problems in yaml files like label for example: 41:81 error line too long (81 > 80 characters) (line-length) where I used this notation to properly format it:

Key: >
  this is my very very very
  long string

From this webpage.

Which the generator could also be updated on. Other than that I just fixed some twig problems the linter exposed so we seem pretty good for now.

Here's the related issue:
https://www.drupal.org/project/demo_design_system/issues/3467234 📌 Use SDC conversion script to create baseline SDC YAML and README files Fixed

Thanks!

@anand.toshniwal93 If you can update the script to handle:

1. README text length <= 80 chars
2. YAML text length <= 80 chars

That would be great. The first one is easiest since they are all the same.

Note:

• We can manually fix these, so let us know if you don't have time to update.
• We are working on more custom components based on new d.o redesign work. We were just informed about these last Thursday.
• We'll need to run the script on these new components later this week or early next week.

I will update script today for above mentioned feedback.

Thanks! I'm closing this one. If necessary, we can have another feedback issue.

Automatically closed - issue fixed for 2 weeks with no activity.