Stepped List component p tag warning error

[albertkol]

The stepped List component surrounds the List item in a p tag which has the validator complain.

Basically almost every HTML element will have the validator complain about. Could we perhaps surround everything in a div instead?

[shdq]

Hello @albertkol. I hope you're doing well!

I just applied to Canonical and decided to look at open-source repos of the company. It seems you need a hand with this, so I looked at the issue and I think you're talking about nested lists.

This code would also produce the same semantic error. So it is not only about the stepped list.

const innerItems = [
  {
    title: "My title",
    content:
      "My not very long sentence with small details.",
  },
];

const outerItems = [
  {
    content: <List items={innerItems} />,
  },
];

<List items={outerItems} />;

I suggest a quick fix for this changing p to div. I think span would fit better, but it won't work because it is inline element and there is no css rule in the corresponding class of vanilla framework (which is in another repo) that makes the element display as block.

I also want to ask for @bartaz thoughts about this issue since he is the main contributor to this repo. And If it is ok, I can help with this issue.

Small feedback:

I reproduced this issue in the sandbox and there are no instructions for how to add styles along with the react-components package. I guess I need also to install vanilla-framework, and sass, build and import styles, but it wasn't needed to reproduce this issue. So it would be great to have clear instructions in this repo about how to have styles out of the box.

I also noticed that vanilla framework API on lists is not fully supported. For example Nested Count, but it is out of the scope of this issue.

[bartaz]

@shdq Hi Sergei, thanks for reaching out and looking into that. I guess Albert went a bit over the top with the labels, but it's definitely bug that would be worth fixing.

I believe the issue mentioned is about the stepped list variant (https://canonical.github.io/react-components/?path=/docs/list--default-story#vertical-stepped). And the issue with wrongly nested tags can happen if you pass multiple p inside the content.

We are likely to look into this ourselves when working on bugfixes in next couple weeks, but if you want to look into that and provide a PR @shdq feel free to do so, and we can assist in getting it reviewed and merged.

As for your other questions:

I reproduced this issue in the sandbox and there are no instructions for how to add styles along with the react-components package. I guess I need also to install vanilla-framework, and sass, build and import styles, but it wasn't needed to reproduce this issue. So it would be great to have clear instructions in this repo about how to have styles out of the box.

I'm not sure exactly what kind of setup you mean, if you want feel free to open a separate issue to describe the setup you tried and what didn't work.
In our case we clone this project and run it through storybook that has the styles included. But I guess if you just install react-components package it indeed would need to have vanilla-framework installed separately (as it's defined in peer dependencies).

I also noticed that vanilla framework API on lists is not fully supported. For example Nested Count, but it is out of the scope of this issue.

Yes, not all components and their variants are currently covered by React implementation. We are adding those gradually when they are needed by some of our React projects.

[shdq]

Hello @bartaz. Thanks for the quick response!

I believe the issue mentioned is about the stepped list variant (https://canonical.github.io/react-components/?path=/docs/list--default-story#vertical-stepped). And the issue with wrongly nested tags can happen if you pass multiple p inside the content.

Got it. I guess for now it is not intended by react implementation to use List as a child of another List. Correct?

We are likely to look into this ourselves when working on bugfixes in next couple weeks, but if you want to look into that and provide a PR @shdq feel free to do so, and we can assist in getting it reviewed and merged.

If you agree with the suggested small fix replacing p with div, I will open a PR.

[ClementChaumel]

We are likely to look into this ourselves when working on bugfixes in next couple weeks, but if you want to look into that and provide a PR @shdq feel free to do so, and we can assist in getting it reviewed and merged.

If you agree with the suggested small fix replacing p with div, I will open a PR.

Hey @shdq I'd be happy to review your PR. Please assign it to me once it's ready!

[shdq]

Hello @ClementChaumel

I opened PR #886 and it is ready for review.

Have a nice day!

[bartaz]

Fixed by #886