Clay: Verify clay-charts for 3.0.0 release

Created on 16 Sep 2019  ·  5Comments  ·  Source: liferay/clay

  • [x] Are you able to easily implement a demo of this component? Are there any “gotchas” when implementing? Provide a link to codesandbox for a demo.
  • [ ] Does the component accept additional HTML attributes? For example, adding an aria attribute to a button.
  • [x] Does component accept a ref? Is the ref value what you would expect?
  • [x] Does this component allow for different translations?
  • [ ] If this is a form element, does it accept a name attribute?
  • [x] Were you able to find documentation for this component? Was it clear?

Additional Questions:

  • Is there anything about the public API that you would change?
  • Is the naming of the available props clear?
3.x clay-components

All 5 comments

Does the component accept additional HTML attributes? For example, adding an aria attribute to a button.

As far as I could see we're not taking this into account ... yet.
As discussed in other "review" issue tickets, it might be better to come up with some common patterns concerning these extra attributes before adding anything we might end up reverting.

Is there anything about the public API that you would change?

Not at the moment.

Is the naming of the available props clear?

As far as I'm concerned that's a "yes", because it's just of matter of reading the billboard.js documentation and examples.

I've also had other people ask me about these charts and their usage (with the previous metal+soy version) and I usually suggested to look at the billboard.js documentation too.
I don't know how to do this in the most efficient way but it would be nice to specify that this is "just" a thin wrapper on top of billboard.js because it sometimes seems that wasn't the expectation. @bryceosterhaus @matuzalemsteles what do you guys think?

Yeah you are right, we could do a better job at pointing to the billboard docs. I haven't look at charts in awhile, but I think we might benefit from going through it one more time and trying to line of the API to be as close to billboard as possible and any unnecessary logic on our end.

@bryceosterhaus yes that sounds like a good idea too - I haven't seen too much unnecessary stuff too far.

@julien I went ahead and added an additional prop for providing attributes to the charts container element. Let me know if this seems appropriate https://github.com/liferay/clay/pull/2514. I didn't use ...otherProps since I wanted to leave that open for other billboard config and API.

@bryceosterhaus yes that looks appropriate to me. Thanks.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

joseigor picture joseigor  ·  5Comments

pat270 picture pat270  ·  4Comments

brunofarache picture brunofarache  ·  5Comments

bryceosterhaus picture bryceosterhaus  ·  4Comments

dgarciasarai picture dgarciasarai  ·  4Comments