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

Refine the Dot Leader list code example #1356

Merged
merged 4 commits into from
Jul 1, 2021

Conversation

gerardo-rodriguez
Copy link
Member

@gerardo-rodriguez gerardo-rodriguez commented Jun 30, 2021

Overview

This PR refines the code example for the Dot Leader list example.

Screenshots

Screen Shot 2021-06-30 at 1 38 00 PM

Testing

Preview: https://deploy-preview-1356--cloudfour-patterns.netlify.app/?path=/docs/components-dot-leader--list

  1. Review the Dot Leader list code example for accuracy

@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2021

⚠️ No Changeset found

Latest commit: f6178df

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@gerardo-rodriguez gerardo-rodriguez self-assigned this Jun 30, 2021
@gerardo-rodriguez gerardo-rodriguez requested review from tylersticka and a team June 30, 2021 21:12
Comment on lines 52 to 65
code: `{% embed '@cloudfour/objects/list/list.twig' only %}
{% block content %}
{% for topic in topics %}
<li>
{% include '@cloudfour/components/dot-leader/dot-leader.twig' with {
label: topic.title,
count: topic.count,
href: topic.url
} only %}
</li>
{% endfor %}
{% endblock %}
{% endembed %}
`,
Copy link
Member

@tylersticka tylersticka Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gerardo-rodriguez Unless I'm mistaken, this looks like it's exactly the contents of the demo/list.twig file.

In cases where you want to show the raw template source, it's possible to do so without copying and pasting code.

Near the beginning of the file, you should see the import for the demo itself:

import listDemo from './demo/list.twig';

Duplicate this line, but give it a unique name and prepend !!raw-loader! to the path:

import listDemo from './demo/list.twig';
import listDemoSource from '!!raw-loader!./demo/list.twig';

Then, you can use listDemoSource (or whatever you called it) as the value of docs.source.code. It should show the contents of the list.twig file as a string.

More info: https://github.com/webpack-contrib/raw-loader#examples

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What!? 🤯

Thanks, @tylersticka, I'll take a look! 🔍 👀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks, @tylersticka! e38b9e6

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh...lint failure... 🔍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was getting a lint error. I disabled it. Not sure if disabling this specific use case is frowned upon. Do you have any insight, @tylersticka?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! Here's the info about the lint rule, and the relevant bit:

This syntax is non-standard, so it couples the code to Webpack. The recommended way to specify Webpack loader configuration is in a Webpack configuration file.

I get the intent here. import is a native feature that works anywhere. By using custom webpack syntax we're breaking that interoperability and tying ourselves to webpack. That said, it seems unlikely that Storybook would move away from webpack any time soon, and since we're already tied to Storybook it seems like we're already tied to webpack?

I think the worst case scenario is that a new Storybook version moves away from webpack and then we'd need to either:

  • Add our own webpack config.
  • Update anywhere we use this syntax to do something else.
  • Not update Storybook.

That doesn't seem all that scary? I'd be more concerned about this lint rule being used in our actual components, but it seems fine for a Storybook docs page?

I'm trying to think through alternatives:

  1. We could update our webpack config so that you could add a query string to specify a specific loader: (e.g. import myRawModule from 'myModule?raw'. I think this could be done using resource query rules?
  2. Write some custom node code to grab a file's contents and spit them out? That seems like reinventing the wheel for no reason and I'm not sure how it would work with Storybook.

It seems like option 1 above might be slightly cleaner since we're not tying ourselves to webpack as directly? But we'd still be relying on webpack to handle the resource query rules. That solution doesn't seem radically better but it sounds more complicated to get working.

My gut feeling is we should leave this rule enabled globally (since we shouldn't be doing this in our component code) but not worry about disabling it in Storybook doc files.

That said I'm curious if @calebeby or @spaceninja have any other thoughts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option 1 feels like we're just recreating a native Webpack feature ourselves for no reason, and option 2 does feel like reinventing the wheel for no reason.

My gut feeling is we should leave this rule enabled globally (since we shouldn't be doing this in our component code) but not worry about disabling it in Storybook doc files.

I agree with this 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @Paul-Hebert — leave the rule enabled, and disable it for places where we're intentionally working around it like Storybook docs.

That said, I'd like to see a comment above the eslint-disable line explaining why we're disabling it. Just a sentence or two summarizing what we've said here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you all! I'll add a comment shortly. 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added! f6178df

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring approval to @cloudfour/dev. This seems a lot more DRY than before, but it's unclear to me what the alternative solution would be to overcome the eslint rule, or if we should be updating our configuration to account for this use case.

@Paul-Hebert Paul-Hebert self-requested a review July 1, 2021 17:10
Paul-Hebert
Paul-Hebert previously approved these changes Jul 1, 2021
@gerardo-rodriguez
Copy link
Member Author

@Paul-Hebert @spaceninja I've updated this PR, can I get a final review/approval? Thanks! 😄

Copy link
Member

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@gerardo-rodriguez gerardo-rodriguez merged commit 9ac52cc into v-next Jul 1, 2021
@gerardo-rodriguez gerardo-rodriguez deleted the chore/dot-leader-source-examples branch July 1, 2021 21:46
gerardo-rodriguez added a commit that referenced this pull request Jul 1, 2021
…into chore/ground-nav-source-examples

* 'v-next' of github.com:cloudfour/cloudfour.com-patterns:
  Refine the Dot Leader list code example (#1356)
gerardo-rodriguez added a commit that referenced this pull request Jul 1, 2021
…into chore/event-log-source-examples

* 'v-next' of github.com:cloudfour/cloudfour.com-patterns:
  Refine the Dot Leader list code example (#1356)
  Comment Form code example + docs + template updates (#1353)
  Add small size of avatar (#1358)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants