accordproject / cicero-ui

A library of React components for Accord Project templates
Apache License 2.0
29 stars 46 forks source link

TemplateLibrary: make styles extensible with classes; change to support heterogeneous libraries #363

Closed petrgazarov closed 4 years ago

petrgazarov commented 4 years ago

Is your feature request related to a problem? Please describe. The TemplateLibrary component fits well into a use case we have at Clause.io, but there are limitations that I hope we can address in this repository.

1) Stylistically, we want to have control over colors, font properties and borders inside the card. 2) Our libraries are heterogeneous and the library items are of various types (not just templates).

Describe the solution you'd like 1) If I can pass class names to the elements inside the component, that would be a big improvement in extensibility. This is a common feature of UI libraries, for example Semantic UI React. Perhaps cicero-ui can expose a default set of styles (same as it currently does), and take classname props instead of props for individual CSS attributes. 2) This component could be agnostic to the type of library items it takes, and prop/component names would reflect that. For example, templates prop is changed to libraryItems, TemplateLibrary export is changed to Library, etc. I'm not sure if this is the direction that cicero-ui is moving, but I thought I'd make this suggestion anyway. Among the benefits is that it widens the potential use cases for this component.

Describe alternatives you've considered An alternative is fork the code and implement changes in our repository.


An example of heterogeneous library, with different styling based on item type.

_HUB-1797__Kits_in_clause_editor_side_nav_-_Jira

jeromesimeon commented 4 years ago

This sounds like a good idea. I don't have a very good understanding of the internals of this class, but I have two questions:

@DianaLease @irmerk any thoughts?

DianaLease commented 4 years ago

I like (1) in @petrgazarov's list and is the direction we are moving toward regardless (see https://github.com/accordproject/cicero-ui/issues/320). I'm also fine with (2). It will be a breaking change but making it generic means we wouldn't run into this issue again in the future if there are other types of library items users would like to display.

DianaLease commented 4 years ago

@irmerk - Quick question for you: @petrgazarov is going to work on these two items in 2 separate PRs. Which branch should be used? master or development? I know we are getting rid of development but right now it is still the default.

irmerk commented 4 years ago
  1. Stylistically, we want to have control over colors, font properties and borders inside the card.
  2. If I can pass class names to the elements inside the component, that would be a big improvement in extensibility. This is a common feature of UI libraries, for example Semantic UI React. Perhaps cicero-ui can expose a default set of styles (same as it currently does), and take classname props instead of props for individual CSS attributes.

I agree with @DianaLease

I like (1) in @petrgazarov's list and is the direction we are moving toward regardless (see #320).

We are moving away from our heavy reliance on styling props. This can be revisited when the Slate 0.57 migration is complete, as I think it will solve most if not all of this issue.


  1. Our libraries are heterogeneous and the library items are of various types (not just templates).
  2. This component could be agnostic to the type of library items it takes, and prop/component names would reflect that. For example, templates prop is changed to libraryItems, TemplateLibrary export is changed to Library, etc. I'm not sure if this is the direction that cicero-ui is moving, but I thought I'd make this suggestion anyway. Among the benefits is that it widens the potential use cases for this component.

I like this change being in line with AP trying to be as agnostic as possible. I'm unsure what the implications will be for addToCont/+ Add to contract, as different library items are likely not going to be "added to the contract". This raises some further questions for me, but this doesn't seem to make those questions worse. Any thoughts on this idea? @Michael-Grover


An alternative is fork the code and implement changes in our repository.

This may be rough in the long term, constantly needing to keep up to date of the rapid changes in this repository.


@irmerk - Quick question for you: @petrgazarov is going to work on these two items in 2 separate PRs. Which branch should be used? master or development? I know we are getting rid of development but right now it is still the default.

I'd say hold off on the styling work until after the Slate migration, as this may be solved by then. Otherwise, stick to development for now to keep it consistent.

Michael-Grover commented 4 years ago
  1. Our libraries are heterogeneous and the library items are of various types (not just templates).
  2. This component could be agnostic to the type of library items it takes, and prop/component names would reflect that. For example, templates prop is changed to libraryItems, TemplateLibrary export is changed to Library, etc. I'm not sure if this is the direction that cicero-ui is moving, but I thought I'd make this suggestion anyway. Among the benefits is that it widens the potential use cases for this component.

I like this change being in line with AP trying to be as agnostic as possible. I'm unsure what the implications will be for addToCont/+ Add to contract, as different library items are likely not going to be "added to the contract". This raises some further questions for me, but this doesn't seem to make those questions worse. Any thoughts on this idea? @Michael-Grover

I'm not sure I follow the question, but I like the idea that different types of library items can look different. Maybe they could have customized buttons too in the future if there are library items that cant' be added to the contract?

irmerk commented 4 years ago

@petrgazarov are you still planning on incorporating this work into the component?