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

WICKET-6941 Add visible attribute to AbstractColumn to hide/show columns in DataTable/DataGridView #491

Open
wants to merge 12 commits into
base: wicket-9.x
Choose a base branch
from

Conversation

RLStokes
Copy link

Not sure about the change to the interface ICellPopulator whether it is allowed in a minor version update.

Copy link
Contributor

@reiern70 reiern70 left a comment

Choose a reason for hiding this comment

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

Not sure I understand the use case for this. A table which dynamically can have more or less columns? I would not mind this change is code is made read only with a default method in interface.

@@ -71,4 +71,8 @@
*/
void populateItem(final Item<ICellPopulator<T>> cellItem, final String componentId,
final IModel<T> rowModel);

boolean isVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this to be be

default boolean isVisible() {
    return true;
}

and no setter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also JavaDoc.


boolean isVisible();

void setVisible(boolean visible);
Copy link
Contributor

Choose a reason for hiding this comment

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

As said I would prefer this to be only read-only

@@ -74,4 +75,16 @@ public void populateItem(final Item<ICellPopulator<T>> cellItem, final String co
{
cellItem.add(new Label(componentId, new PropertyModel<>(rowModel, property)));
}

@Override
public boolean isVisible()
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment above.

@@ -37,6 +37,8 @@

private final S sortProperty;

private boolean visible = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment about read only

@@ -71,7 +71,10 @@

for (IColumn<T, S> column : table.getColumns())
{
columnsModels.add(Model.of(column));
if (column.isVisible())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the only involved toolbar that needs to be fixed... Did you run

mvn clean install

?

@reiern70
Copy link
Contributor

And thanks for your work :-)

@martin-g martin-g closed this Apr 27, 2022
@martin-g martin-g reopened this Apr 27, 2022
@martin-g
Copy link
Member

Close/reopen to re-trigger the build checks.

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