[ChipGroup] Allow the option to set max rows of chips to display for a chip group #1071

bennycao · 2020-02-29T07:30:55Z

Partially solves #953

Add ChipGroup option to set maxRowCount. If set, will display chips up to maxRowCount.

Add ChipGroup option overflowChipEnabled and overflowChipTextResource. If enabled with maxRowCount will display a chip using overflowChipTextResource (can be a PluralRes or StringRes) to set the text to display when reflow exceeds maxRowCount.

Added a new catalog for chipgroup showing example of usage.

The use case for enabling overflowChipEnabled or even setting maxRowCount works well for display of read only (no interactions) chips. The implementation of this could include binding a onClick event on the overflow chip to navigate to an edit/selection type page of chips

Identify the component the PR relates to in brackets in the title.
[Buttons] Updated documentation
Link to GitHub issues it solves. Partially solves https://github.com/material-components/material-components-android/issues/953
Sign the CLA bot. You can do this once the pull request is opened.

wcshi · 2020-03-03T18:02:10Z

It's great to see that you have a chip showing how many more chips are available, however, I think it would make sense if it was on the 2nd row instead of an additional row.

bennycao · 2020-03-03T21:48:59Z

It's great to see that you have a chip showing how many more chips are available, however, I think it would make sense if it was on the 2nd row instead of an additional row.

I think having it on the 2nd or new row can depend on the design requirements. This possibly could be a choice as opposed to a rule ?

wcshi · 2020-03-05T16:34:20Z

Could you please add an overflowChipSingularResource as well?

bennycao · 2020-03-05T22:41:13Z

Could you please add an overflowChipSingularResource as well?

👍 . Is this so we can have view text that just displays more... or the like ?

bennycao · 2020-03-06T11:27:25Z

Could you please add an overflowChipSingularResource as well?

@wcshi updated so that the attribute overflowChipPluralResource is now overflowChipTextResource and can accept a PluralRes or StringRes

wcshi

This is not a comprehensive set of comments, there are still problems, we are considering opening up the api so this could be an extension instead.

wcshi · 2020-03-05T21:55:09Z

lib/java/com/google/android/material/internal/FlowLayout.java

@@ -144,6 +180,12 @@ protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
      // not confined to a single line, move this child to the next line and reset its left bound to
      // flowlayout's left bound.
      if (childRight > maxRight && !isSingleLine()) {
+        rowCount++;
+
+        if (this.maxRowCount > 1 && this.maxRowCount <= rowCount) {


Why is this > 1 instead of 0? While a bit silly, a user could set maxRowCount to 1 and not want them to scroll off the screen.

yeah, good point this was an oversight

wcshi · 2020-03-05T22:07:36Z

lib/java/com/google/android/material/chip/res/values/attrs.xml

+    <attr name="maxRowCount" format="integer"/>
+    <attr name="overflowChipEnabled" format="boolean"/>
+    <attr name="overflowChipPluralResource" format="reference"/>


Please add these attributes to public.xml

https://github.com/material-components/material-components-android/blob/master/lib/java/com/google/android/material/chip/res-public/values/public.xml

wcshi · 2020-03-05T22:15:51Z

lib/java/com/google/android/material/internal/FlowLayout.java

    }

    maxChildRight += getPaddingRight();
    childBottom += getPaddingBottom();

+    // if we have remaining child views, adjust height to enable showing the `X more child` view on


s/we have remaining/there are more

wcshi · 2020-03-05T22:17:19Z

catalog/java/io/material/catalog/chip/ChipGroupMaxRowsDemoFragment.java

@@ -0,0 +1,89 @@
+/*
+ * Copyright 2017 The Android Open Source Project


wcshi · 2020-03-05T22:27:13Z

catalog/java/io/material/catalog/chip/ChipGroupMaxRowsDemoFragment.java

+/** A fragment that displays the ChipGroup Max row demos for the Catalog app. */
+public class ChipGroupMaxRowsDemoFragment extends DemoFragment {


This demo is a bit confusing

doesn't explicitly state the max row count

none of the chips are checkable but they are not disabled

there's FAB but it doesn't do anything

Clicking on the 7 more cities chip doesn't expand to show the rest of the chips

It would be great if the demo displayed and let the user adjust the maxRowCount.

Max row count is defined in the chipgroup demo layout.

Chips are displayed using a layout that makes the chips not selectable i.e. cat_chip_group_item_entry.xml

Yep, fab shouldn't be there

I didn't want to force the behaviour on what clicking on the 7 more cities chip to do. This i think should be a decision of the implementer. Possibly define a onClick listener for the overflow chip ?

wcshi · 2020-03-06T15:54:09Z

lib/java/com/google/android/material/chip/ChipGroup.java

@@ -135,6 +137,9 @@ public ChipGroup(Context context, AttributeSet attrs, int defStyleAttr) {
    setChipSpacingVertical(
        a.getDimensionPixelOffset(R.styleable.ChipGroup_chipSpacingVertical, chipSpacing));
    setSingleLine(a.getBoolean(R.styleable.ChipGroup_singleLine, false));
+    setMaxRowCount(a.getInt(R.styleable.ChipGroup_maxRowCount, 0));
+    setOverflowChipEnabled(a.getBoolean(R.styleable.ChipGroup_overflowChipEnabled, false));


Under what circumstances do we want this to be false?

If it's decided that you never can have a maxRowCount defined and not show a overflow chip then, no, it could never be false. Is this something that should be a standard or not optional ?

wcshi · 2020-03-06T15:56:35Z

lib/java/com/google/android/material/internal/FlowLayout.java

+      View child = this.getChildAt(this.getChildCount() - 1);
+
+      if (child.getVisibility() != View.GONE) {
+        // need to assign TextView text here, otherwise measurement of view width will be wrong


What does this mean?

Reasoning behind this is, if the overflow chip is to be displayed on the same row as other chips, and fits, we need to know what the width is, hence adding the resource text to the chip. Which then we can have a correct measure of the overflow chip. And whether reflow is required.

wcshi · 2020-03-06T15:57:38Z

lib/java/com/google/android/material/internal/FlowLayout.java

+        } else if (getResourceNameTypeName(this.overflowChildTextResource).equals("string")) {
+          ((TextView) child).setText(getContext().getResources().getString(this.overflowChildTextResource));
+        } else {
+          throw new InvalidParameterException("overflowChildTextResource can only be a plural or string resource");


Validation during on measure is expensive and unnecessary, please move this into the setter.

wcshi · 2020-03-06T15:58:45Z

lib/java/com/google/android/material/internal/FlowLayout.java

+                          this.overflowChildTextResource, remainingItems, remainingItems
+                  )
+          );
+        } else if (getResourceNameTypeName(this.overflowChildTextResource).equals("string")) {


On second thought, the api should require/only support a PluralRes so that the user supplies the appropriate unit for singular or plural left over child views.

wcshi · 2020-03-06T16:03:53Z

lib/java/com/google/android/material/internal/FlowLayout.java

+    // if we have remaining child views, adjust height to enable showing the `X more child` view on
+    // next line
+    if (this.overflowChildEnabled && remainingItems > 0) {
+      View child = this.getChildAt(this.getChildCount() - 1);


This alters the text of the last child view so that the user cannot display the original context, this is not okay.

bennycao · 2020-03-07T12:35:29Z

This is not a comprehensive set of comments, there are still problems, we are considering opening up the api so this could be an extension instead.

This is a starting point on what direction we want to take it.

As i see it, i don't see a usecase where we would want to limit the display rows and have any chips selectable when you don't show all the chips available. Hence ideally the only reason you would have a max row count is in a read only state. And the implementer can choose to decide how they want to proceed to a editable/selectable like display. This can be a new screen or the re-render of the chipgroup without a max row defined.

Will take your lead on this on how to proceed on this feature. Happy to help where i can.

madCode · 2020-09-30T04:48:39Z

Any chance this will be resolved and merged soon? Super excited for the option to set a max number of rows and have the ChipGroup know how to scroll from there. I'd like to display a bunch of actionable chips that a user can click, but want more than just one row of chips. I'd rather have a scrollable grid with a customizable number of rows of chips.

…h accepts a stringres or pluralres)

…rce which accepts a stringres or pluralres)" This reverts commit 999d5bb.

ahulyk · 2021-04-26T12:18:03Z

any progress on that?

luangs7 · 2022-08-04T16:53:52Z

any progress on that?

bennycao · 2022-08-11T22:58:43Z

Hi all, sorry for such a delay on my side on responding to the interest of this feature. As it seems, there doesn't seem to be an appetite from their end to have this feature as a feature on the materials component. This has been open for a long time and not sure if google are willing to revisit. There is also what looks like similar issue/request #696 which has a long timeline.

michaellee123 · 2023-06-02T08:27:16Z

I'm sad that Google does not merge this pr. Many times setting the max line is very useful. You did a good job! 👍

googlebot added the cla: yes label Feb 29, 2020

wcshi reviewed Mar 6, 2020

View reviewed changes

wcshi added the Work in progress (WIP) PR label Mar 25, 2020

bennycao added 13 commits December 11, 2020 14:35

add support for max rows on chipgroup

05b3141

create new chip group max row demo

ed2c8f5

fix display of x chips remaining

04b5820

use TextView to settext

2f77842

remove single selection switch

9a00efd

refine measure of last x more chip

1b15a1f

remove redundant layout

15a2c07

add accessor methods

e0beaa2

change overflowChipPluralResource to be overflowChipTextResource whic…

a1142b9

…h accepts a stringres or pluralres)

Revert "change overflowChipPluralResource to be overflowChipTextResou…

0461aa9

…rce which accepts a stringres or pluralres)" This reverts commit 999d5bb.

layout cleanup

eb26089

fix when maxrowcount = 1

9e222d1

fix after rebase

ca1f179

bennycao force-pushed the feature/chip-multi-line branch from f7812b0 to ca1f179 Compare December 11, 2020 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ChipGroup] Allow the option to set max rows of chips to display for a chip group #1071

[ChipGroup] Allow the option to set max rows of chips to display for a chip group #1071

bennycao commented Feb 29, 2020 •

edited

Loading

wcshi commented Mar 3, 2020

bennycao commented Mar 3, 2020

wcshi commented Mar 5, 2020

bennycao commented Mar 5, 2020

bennycao commented Mar 6, 2020

wcshi left a comment

wcshi Mar 5, 2020

bennycao Mar 7, 2020

wcshi Mar 5, 2020

wcshi Mar 5, 2020

wcshi Mar 5, 2020

wcshi Mar 5, 2020

bennycao Mar 7, 2020 •

edited

Loading

wcshi Mar 6, 2020

bennycao Mar 7, 2020

wcshi Mar 6, 2020

bennycao Mar 7, 2020 •

edited

Loading

wcshi Mar 6, 2020

wcshi Mar 6, 2020

wcshi Mar 6, 2020

bennycao commented Mar 7, 2020 •

edited

Loading

madCode commented Sep 30, 2020 •

edited

Loading

ahulyk commented Apr 26, 2021

luangs7 commented Aug 4, 2022

bennycao commented Aug 11, 2022

michaellee123 commented Jun 2, 2023

		@@ -0,0 +1,89 @@
		/*
		* Copyright 2017 The Android Open Source Project

		/** A fragment that displays the ChipGroup Max row demos for the Catalog app. */
		public class ChipGroupMaxRowsDemoFragment extends DemoFragment {

[ChipGroup] Allow the option to set max rows of chips to display for a chip group #1071

Are you sure you want to change the base?

[ChipGroup] Allow the option to set max rows of chips to display for a chip group #1071

Conversation

bennycao commented Feb 29, 2020 • edited Loading

wcshi commented Mar 3, 2020

bennycao commented Mar 3, 2020

wcshi commented Mar 5, 2020

bennycao commented Mar 5, 2020

bennycao commented Mar 6, 2020

wcshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennycao Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennycao Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennycao commented Mar 7, 2020 • edited Loading

madCode commented Sep 30, 2020 • edited Loading

ahulyk commented Apr 26, 2021

luangs7 commented Aug 4, 2022

bennycao commented Aug 11, 2022

michaellee123 commented Jun 2, 2023

bennycao commented Feb 29, 2020 •

edited

Loading

bennycao Mar 7, 2020 •

edited

Loading

bennycao Mar 7, 2020 •

edited

Loading

bennycao commented Mar 7, 2020 •

edited

Loading

madCode commented Sep 30, 2020 •

edited

Loading