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

improv: allow defining limits for elastic grid #14

Merged
merged 7 commits into from
Jun 8, 2022

Conversation

nfsxreloader
Copy link
Contributor

@nfsxreloader nfsxreloader commented Jun 7, 2022

Context

Allows defining the max amount of columns or rows that the elastic grid can have.

Approach

Allows to settle maxElasticColumns and maxElasticRows on SwayzeTableDataController.

@nfsxreloader nfsxreloader marked this pull request as ready for review June 7, 2022 14:28
Copy link
Contributor

@bernardobelchior bernardobelchior 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 to me 👍

Two additional things:

  1. Do we want to add this change to the changelog now or later?
  2. Do we want to add this some docs so that it becomes clear that we now support this?

final maxPosition = elasticCount != null
// in case the user has settled a max elastic count, we should
// limit the grid expansion to that count, however, if that limit
// is lower than the table size, we should prioritize the table size
Copy link
Contributor

Choose a reason for hiding this comment

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

if that limit is lower than the table size, we should prioritize the table size over it.

Shouldn't it always be lower than maxElasticCount? In which case does it make sense to have a table bigger than maxElasticCount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's more of a safe check, there shouldn't be a use case for that

@nfsxreloader nfsxreloader force-pushed the improv/allow-limiting-elastic-grid branch from 93939c6 to b1158db Compare June 8, 2022 08:56
@nfsxreloader
Copy link
Contributor Author

nfsxreloader commented Jun 8, 2022

Do we want to add this some docs so that it becomes clear that we now support this?

Maybe when we merge to main we update the README.md also with drag and drop and resize?

Or when we add the example app

@bernardobelchior
Copy link
Contributor

Maybe when we merge to main we update the README.md also with drag and drop and resize?

That's fine by me, as long as we create an issue to track it 😄

@nfsxreloader nfsxreloader merged commit 7a4714c into develop Jun 8, 2022
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.

3 participants