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

Component refactoring #34

Closed
javier-godoy opened this issue Sep 30, 2020 · 4 comments
Closed

Component refactoring #34

javier-godoy opened this issue Sep 30, 2020 · 4 comments

Comments

@javier-godoy
Copy link
Member

In #6 we made the internal grids protected instead of private, so that they can be configured by extending the component (they were not made public in order to conceal the data provider from the public API, however increasing their visibility is still an option).

Since grids are not public, feature #9 required adding SIX methods to the public API (three for left-delegation and three for right-delegation):

  • setLeftGridClassName / setRightGridClassName that delegate on left/right setClassName
  • addLeftGridClassName / addRightGridClassName that delegate on left/right addClassName
  • removeLeftGridClassName / removeRightGridClassName that delegate on left/right removeClassName

This (anti)pattern was not noticeable when there was only a single pair of withLeftColumnCaption / withRightColumnCaption methods, but the evolution of the component seems to be guided by chirality. #25 attempts to address this ambidextrous code duplication on the component internals, but the public API still suffers from it.

On the other hand (pun intended) addColumn is cool-fluent, thus #8 had to introduce addSortableColumn(Book::getIsbn, comparing(Book::getIsbn), "ISBN") when addColumn(Book::getIsbn, "ISBN").setComparator(comparing(Book::getIsbn)) would have sufficed. There are still many other settings such as classNameGenerator. resizable, flexGrow and key, that cannot be configured through public API.

Latest PR #33 exposes this situation up to the point where a refactoring has to be considered.

Any thoughts?

@mlopezFC
Copy link
Member

mlopezFC commented Oct 9, 2020

Good point.

The following things pops into my mind. Maybe we should consider that this component was meant to be a better replacement of List Builder, giving the features of dealing with the cases were more data is needed for displaying a lot of items (columns, filtering, sorting, etc.). Taking that into account, now I think that giving the possibility of customizing each of the grids individually goes beyond the idea of the original use case, because if you want to move around data from two different looking grids is maybe another different component.

Thinking in that direction, those methods that we added to configure the styles of each grid, could have been 3 instead of 6, and simply to add a suffix for each of the class names of the internal grids ("-left" and " -right ") would do the trick with a smaller API.

So basically I think that giving the possibility of customizing each of the grids should be something achievable only by extending the component, but the API should remain clean for the original purpose of the component (basically almost every customization should apply to each of the grids at the same time).

@javier-godoy javier-godoy added this to Inbox (needs triage) in Flowing Code Addons (legacy) Apr 27, 2021
@mlopezFC mlopezFC moved this from Inbox (needs triage) to To Do in Flowing Code Addons (legacy) Apr 19, 2022
@javier-godoy javier-godoy self-assigned this Apr 19, 2022
@javier-godoy javier-godoy moved this from To Do to Inbox (needs triage) in Flowing Code Addons (legacy) Jun 21, 2022
@javier-godoy
Copy link
Member Author

Back to triage.

I think that giving the possibility of customizing each of the grids should be something achievable only by extending the component.

I disagree. There is and/or there should be public API for customizing the component.

@javier-godoy
Copy link
Member Author

Unassigning myself because this issue still needs triage.

@javier-godoy
Copy link
Member Author

Closing in favor of #108 and #116.

Flowing Code Addons (legacy) automation moved this from Inbox (needs triage) to Done Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants