You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by GitBox <gi...@apache.org> on 2021/05/28 07:22:56 UTC
[GitHub] [wicket] rkurucz9 opened a new pull request #471: WICKET-6888 ToggleableDataTable
rkurucz9 opened a new pull request #471:
URL: https://github.com/apache/wicket/pull/471
I would like to add a ToggleableDataTable to the codebase. Each row of it has an expansion content.
If I have to fix my code, please share it with me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] rkurucz9 commented on a change in pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
rkurucz9 commented on a change in pull request #471:
URL: https://github.com/apache/wicket/pull/471#discussion_r641475471
##########
File path: wicket-extensions/src/main/java/org/apache/wicket/extensions/markup/html/repeater/data/table/toggle/ToggleColumn.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.extensions.markup.html.repeater.data.table.toggle;
+
+import org.apache.wicket.ClassAttributeModifier;
+import org.apache.wicket.Component;
+import org.apache.wicket.extensions.markup.html.repeater.data.grid.ICellPopulator;
+import org.apache.wicket.extensions.markup.html.repeater.data.table.AbstractColumn;
+import org.apache.wicket.extensions.markup.html.repeater.data.table.DataTable;
+import org.apache.wicket.extensions.markup.html.repeater.data.toggle.IToggleable;
+import org.apache.wicket.markup.html.link.Link;
+import org.apache.wicket.markup.repeater.Item;
+import org.apache.wicket.model.IModel;
+import org.apache.wicket.model.Model;
+
+import java.util.Set;
+
+/**
+ * A column displaying the toggleable rows.
+ *
+ * @param <T> The model object type
+ * @param <S> the type of the sorting parameter
+ * @author Roland Kurucz
+ */
+public class ToggleColumn<T, S> extends AbstractColumn<T, S> {
+
+ private IToggleable<T> dataTable;
+
+ public ToggleColumn(final IModel<String> displayModel) {
+ super(displayModel);
+ }
+
+ @Override
+ public void populateItem(final Item<ICellPopulator<T>> cellItem, final String componentId, final IModel<T> rowModel) {
+ final Component toggleComponent = newToggleComponent(componentId, rowModel);
+ toggleComponent.add(new ClassAttributeModifier() {
+ @Override
+ protected Set<String> update(Set<String> oldClasses) {
+ final T object = rowModel.getObject();
+ if (dataTable.isToggleable(object)) {
+ oldClasses.add("toggleable");
Review comment:
Would I fix the other CSS classes (collapsible, expandable) in the ToggleColumn?
Would I fix it in the ToggleableDataTable class for the "expansion-row" CSS class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] svenmeier commented on pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
svenmeier commented on pull request #471:
URL: https://github.com/apache/wicket/pull/471#issuecomment-851249933
I've taken a second look and actually your solution is very good. You clearly have understood how to work with components :).
My major grieve is how the collection of expanded objects is held in the component tree: this defies the 'holy mantra of detachment', i.e. the selected entities will get serialized along with the page. You could take a look at AbstractTree#newModel() for inspiration how this can be avoided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] rkurucz9 commented on pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
rkurucz9 commented on pull request #471:
URL: https://github.com/apache/wicket/pull/471#issuecomment-854116236
> I've taken a second look and actually your solution is very good. You clearly have understood how to work with components :).
>
> My major grieve is how the collection of expanded objects is held in the component tree: this defies the 'holy mantra of detachment', i.e. the selected entities will get serialized along with the page. You could take a look at AbstractTree#newModel() for inspiration how this can be avoided.
I hope I could fix it properly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] rkurucz9 commented on a change in pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
rkurucz9 commented on a change in pull request #471:
URL: https://github.com/apache/wicket/pull/471#discussion_r641899457
##########
File path: wicket-examples/src/main/java/org/apache/wicket/examples/repeater/repeater.css
##########
@@ -72,3 +72,16 @@ col.yellow {
col.brown {
background-color: #CC6633;
}
+
+.toggleable {
Review comment:
I used a more specific CSS class rule for toggleable, collapsible, and expandable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] rkurucz9 commented on pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
rkurucz9 commented on pull request #471:
URL: https://github.com/apache/wicket/pull/471#issuecomment-851608871
> I've taken a second look and actually your solution is very good. You clearly have understood how to work with components :).
>
> My major grieve is how the collection of expanded objects is held in the component tree: this defies the 'holy mantra of detachment', i.e. the selected entities will get serialized along with the page. You could take a look at AbstractTree#newModel() for inspiration how this can be avoided.
Thank you for your praising and suggestion. :) I try to fix my solution according to the AbstractTree#newModel() aspect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] rkurucz9 commented on pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
rkurucz9 commented on pull request #471:
URL: https://github.com/apache/wicket/pull/471#issuecomment-850789089
It's all right from my side. I agree with your concept and arguments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] svenmeier commented on pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
svenmeier commented on pull request #471:
URL: https://github.com/apache/wicket/pull/471#issuecomment-850787702
I've build a lot of these components over the years for different projects, and each had to be different according to the needs of the current project.
There are a whole lot of issues with this implementation, from less important ones like documentation and coding style to larger conceptual errors, performance considerations and code duplication.
I'd rather not pull this code chunk into Wicket as is, and restructuring it might take a long time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] rkurucz9 commented on pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
rkurucz9 commented on pull request #471:
URL: https://github.com/apache/wicket/pull/471#issuecomment-854116236
> I've taken a second look and actually your solution is very good. You clearly have understood how to work with components :).
>
> My major grieve is how the collection of expanded objects is held in the component tree: this defies the 'holy mantra of detachment', i.e. the selected entities will get serialized along with the page. You could take a look at AbstractTree#newModel() for inspiration how this can be avoided.
I hope I could fix it properly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] reiern70 commented on pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
reiern70 commented on pull request #471:
URL: https://github.com/apache/wicket/pull/471#issuecomment-850792867
> I've build a lot of these components over the years for different projects, and each had to be different according to the needs of the current project.
>
> There are a whole lot of issues with this implementation, from less important ones like documentation and coding style to larger conceptual errors, performance considerations and code duplication.
>
> I'd rather not pull this code chunk into Wicket as is, and restructuring it might take a long time.
I fully agree. IMHO. this does not belong to core wicket. At least not in its current state. If cleaned, interfaces and concepts clearly defined it can be maybe added to wicket-stuff. I also had to create similar components for customer projects and I do not think it is that easy to roll a "fit all use cases component".
**Side note**: when creating these kinds of PR it is very useful to attach some images thus that reviewers can easily see how this component looks like.
Thanks for your efforts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #471:
URL: https://github.com/apache/wicket/pull/471#discussion_r641454216
##########
File path: wicket-extensions/src/main/java/org/apache/wicket/extensions/markup/html/repeater/data/table/toggle/ToggleColumn.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.extensions.markup.html.repeater.data.table.toggle;
+
+import org.apache.wicket.ClassAttributeModifier;
+import org.apache.wicket.Component;
+import org.apache.wicket.extensions.markup.html.repeater.data.grid.ICellPopulator;
+import org.apache.wicket.extensions.markup.html.repeater.data.table.AbstractColumn;
+import org.apache.wicket.extensions.markup.html.repeater.data.table.DataTable;
+import org.apache.wicket.extensions.markup.html.repeater.data.toggle.IToggleable;
+import org.apache.wicket.markup.html.link.Link;
+import org.apache.wicket.markup.repeater.Item;
+import org.apache.wicket.model.IModel;
+import org.apache.wicket.model.Model;
+
+import java.util.Set;
+
+/**
+ * A column displaying the toggleable rows.
+ *
+ * @param <T> The model object type
+ * @param <S> the type of the sorting parameter
+ * @author Roland Kurucz
+ */
+public class ToggleColumn<T, S> extends AbstractColumn<T, S> {
+
+ private IToggleable<T> dataTable;
+
+ public ToggleColumn(final IModel<String> displayModel) {
+ super(displayModel);
+ }
+
+ @Override
+ public void populateItem(final Item<ICellPopulator<T>> cellItem, final String componentId, final IModel<T> rowModel) {
+ final Component toggleComponent = newToggleComponent(componentId, rowModel);
+ toggleComponent.add(new ClassAttributeModifier() {
+ @Override
+ protected Set<String> update(Set<String> oldClasses) {
+ final T object = rowModel.getObject();
+ if (dataTable.isToggleable(object)) {
+ oldClasses.add("toggleable");
Review comment:
The CSS class names should be constructed with `CssUtils.key(Class scope, String facet)` helper method.
See https://github.com/apache/wicket/blob/8bbf158ebc4d12f3b317c0d607200441c0767162/wicket-core/src/main/java/org/apache/wicket/markup/html/form/AutoLabelResolver.java#L78-L79 for example.
This way the chance to conflict with application defined CSS classes is much smaller.
##########
File path: wicket-examples/src/main/java/org/apache/wicket/examples/repeater/repeater.css
##########
@@ -72,3 +72,16 @@ col.yellow {
col.brown {
background-color: #CC6633;
}
+
+.toggleable {
Review comment:
See my comment about CSS class names below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #471:
URL: https://github.com/apache/wicket/pull/471#discussion_r641483864
##########
File path: wicket-extensions/src/main/java/org/apache/wicket/extensions/markup/html/repeater/data/table/toggle/ToggleColumn.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.extensions.markup.html.repeater.data.table.toggle;
+
+import org.apache.wicket.ClassAttributeModifier;
+import org.apache.wicket.Component;
+import org.apache.wicket.extensions.markup.html.repeater.data.grid.ICellPopulator;
+import org.apache.wicket.extensions.markup.html.repeater.data.table.AbstractColumn;
+import org.apache.wicket.extensions.markup.html.repeater.data.table.DataTable;
+import org.apache.wicket.extensions.markup.html.repeater.data.toggle.IToggleable;
+import org.apache.wicket.markup.html.link.Link;
+import org.apache.wicket.markup.repeater.Item;
+import org.apache.wicket.model.IModel;
+import org.apache.wicket.model.Model;
+
+import java.util.Set;
+
+/**
+ * A column displaying the toggleable rows.
+ *
+ * @param <T> The model object type
+ * @param <S> the type of the sorting parameter
+ * @author Roland Kurucz
+ */
+public class ToggleColumn<T, S> extends AbstractColumn<T, S> {
+
+ private IToggleable<T> dataTable;
+
+ public ToggleColumn(final IModel<String> displayModel) {
+ super(displayModel);
+ }
+
+ @Override
+ public void populateItem(final Item<ICellPopulator<T>> cellItem, final String componentId, final IModel<T> rowModel) {
+ final Component toggleComponent = newToggleComponent(componentId, rowModel);
+ toggleComponent.add(new ClassAttributeModifier() {
+ @Override
+ protected Set<String> update(Set<String> oldClasses) {
+ final T object = rowModel.getObject();
+ if (dataTable.isToggleable(object)) {
+ oldClasses.add("toggleable");
Review comment:
Yes, please!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] rkurucz9 commented on a change in pull request #471: WICKET-6888 ToggleableDataTable
Posted by GitBox <gi...@apache.org>.
rkurucz9 commented on a change in pull request #471:
URL: https://github.com/apache/wicket/pull/471#discussion_r641527364
##########
File path: wicket-extensions/src/main/java/org/apache/wicket/extensions/markup/html/repeater/data/table/toggle/ToggleColumn.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.extensions.markup.html.repeater.data.table.toggle;
+
+import org.apache.wicket.ClassAttributeModifier;
+import org.apache.wicket.Component;
+import org.apache.wicket.extensions.markup.html.repeater.data.grid.ICellPopulator;
+import org.apache.wicket.extensions.markup.html.repeater.data.table.AbstractColumn;
+import org.apache.wicket.extensions.markup.html.repeater.data.table.DataTable;
+import org.apache.wicket.extensions.markup.html.repeater.data.toggle.IToggleable;
+import org.apache.wicket.markup.html.link.Link;
+import org.apache.wicket.markup.repeater.Item;
+import org.apache.wicket.model.IModel;
+import org.apache.wicket.model.Model;
+
+import java.util.Set;
+
+/**
+ * A column displaying the toggleable rows.
+ *
+ * @param <T> The model object type
+ * @param <S> the type of the sorting parameter
+ * @author Roland Kurucz
+ */
+public class ToggleColumn<T, S> extends AbstractColumn<T, S> {
+
+ private IToggleable<T> dataTable;
+
+ public ToggleColumn(final IModel<String> displayModel) {
+ super(displayModel);
+ }
+
+ @Override
+ public void populateItem(final Item<ICellPopulator<T>> cellItem, final String componentId, final IModel<T> rowModel) {
+ final Component toggleComponent = newToggleComponent(componentId, rowModel);
+ toggleComponent.add(new ClassAttributeModifier() {
+ @Override
+ protected Set<String> update(Set<String> oldClasses) {
+ final T object = rowModel.getObject();
+ if (dataTable.isToggleable(object)) {
+ oldClasses.add("toggleable");
Review comment:
Fixed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org