Implement selection decoration of the main menu. [chromium/src : main]
Hidehiko Abe (Gerrit)
Hidehiko Abe added 2 comments![]( "Open in Gerrit")
var typedValue = new android.util.TypedValue();
var theme = getContext().getTheme();
theme.resolveAttribute(R.attr.colorSecondary, typedValue, true);
int color = typedValue.data;note: as commented in the bug, this part may need to be updated depending on UX reply.
Related details
- Gazal Agarwal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 2 comments![]( "Open in Gerrit")
R+=Wenyu for OWNER.
Now ready for review, as color issue was resolved. PTAL.
var typedValue = new android.util.TypedValue();
var theme = getContext().getTheme();
theme.resolveAttribute(R.attr.colorSecondary, typedValue, true);
int color = typedValue.data;note: as commented in the bug, this part may need to be updated depending on UX reply.
- Gazal Agarwal
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gazal Agarwal (Gerrit)
Gazal Agarwal added 1 comment![]( "Open in Gerrit")
if (mMultiColumnSettings != null) {
mSelectionDecoration = new SelectionDecoration();
getListView().addItemDecoration(mSelectionDecoration);
}Most of logic already exists in `ContainmentItemDecoration.java`. We've implemented a MVC pattern that allows custom styling. Please reuse the same.
Two things to consider:
- The background style will always be a `CARD` for you use case (ContainmentItem.java#BackgroundStyle)
- The custom background color can be set be overriding `ContainerStyle.java#getBackgroundColor`
Related details
- Hidehiko Abe
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 3 comments![]( "Open in Gerrit")
if (mMultiColumnSettings != null) {
mSelectionDecoration = new SelectionDecoration();
getListView().addItemDecoration(mSelectionDecoration);
}Most of logic already exists in `ContainmentItemDecoration.java`. We've implemented a MVC pattern that allows custom styling. Please reuse the same.
Two things to consider:
- The background style will always be a `CARD` for you use case (ContainmentItem.java#BackgroundStyle)
- The custom background color can be set be overriding `ContainerStyle.java#getBackgroundColor`
+1
return null;FWIW this is quite prone to error - for fragment that does not have this method override, the highlight will not be applied.
This also require all the sub-settings to have the correct override, and can be tricky. For example, "Tabs" setting can be on the top-level setting, or it can live within "Appearance" settings (when flag enabled)
return "content_settings";nit: Can we define constants / intDefs for these instead of using string literals?
Related details
- Hidehiko Abe
Hidehiko Abe (Gerrit)
Hidehiko Abe added 2 comments![]( "Open in Gerrit")
if (mMultiColumnSettings != null) {
mSelectionDecoration = new SelectionDecoration();
getListView().addItemDecoration(mSelectionDecoration);
}Wenyu FuMost of logic already exists in `ContainmentItemDecoration.java`. We've implemented a MVC pattern that allows custom styling. Please reuse the same.
Two things to consider:
- The background style will always be a `CARD` for you use case (ContainmentItem.java#BackgroundStyle)
- The custom background color can be set be overriding `ContainerStyle.java#getBackgroundColor`
+1
ContainmentItemDecoration cannot be used here, because it is for containment, i.e.,
- each item in the container will be have background always.
- the rounded corner is applied only for the top and bottom.
- it does not have "selected" state
However, we need a custom implemention.
- We need the background only for selected item.
- we need rounded corners for each item.
We can edit ContainmentItemDecoration to put two different implementation in one decoration, and I wouldn't block the path specifically in the future when we may want to apply more containment-style to the main menu as we are on discussion, but for now, splitting the logic looks what we want?
Or, do you have better ideas, could you share the details with me?
return null;FWIW this is quite prone to error - for fragment that does not have this method override, the highlight will not be applied.
This also require all the sub-settings to have the correct override, and can be tricky. For example, "Tabs" setting can be on the top-level setting, or it can live within "Appearance" settings (when flag enabled)
This also require all the sub-settings to have the correct override, and can be tricky. For example, "Tabs" setting can be on the top-level setting, or it can live within "Appearance" settings (when flag enabled)
We only need to override this on the fragment that is "top detailed page", and it will be in effect only when the fragment is at the bottom of the stack.
So, if "Appearance" is enabled, and Tabs setting is placed under the page, "Appearance" will be only taken care by MainSettings, but "Tabs" will not even if it is put the backstack on top of Appearance.
If tabs is directly opened, indeed it will be the bottom of the stack, but it wouldn't be problematic, because MainSettings won't have the item anyways.
Related details
- Gazal Agarwal
- Wenyu Fu
Gazal Agarwal (Gerrit)
Gazal Agarwal added 1 comment![]( "Open in Gerrit")
if (mMultiColumnSettings != null) {
mSelectionDecoration = new SelectionDecoration();
getListView().addItemDecoration(mSelectionDecoration);
}Wenyu FuMost of logic already exists in `ContainmentItemDecoration.java`. We've implemented a MVC pattern that allows custom styling. Please reuse the same.
Two things to consider:
- The background style will always be a `CARD` for you use case (ContainmentItem.java#BackgroundStyle)
- The custom background color can be set be overriding `ContainerStyle.java#getBackgroundColor`
Hidehiko Abe+1
ContainmentItemDecoration cannot be used here, because it is for containment, i.e.,
- each item in the container will be have background always.
- the rounded corner is applied only for the top and bottom.
- it does not have "selected" state
However, we need a custom implemention.
- We need the background only for selected item.
- we need rounded corners for each item.
We can edit ContainmentItemDecoration to put two different implementation in one decoration, and I wouldn't block the path specifically in the future when we may want to apply more containment-style to the main menu as we are on discussion, but for now, splitting the logic looks what we want?
Or, do you have better ideas, could you share the details with me?
Containment logic is quite flexible and should be able to handle this use case.
You can use BackgroundStyle.NONE to avoid having a background.
BackgroundStyle.CARD would take care of the rounded corners on all ends.
Override `getCustomBackgroundStyle` -> for a custom background style
Override `getCustomBackgroundColor` -> for a custom background color
You can look for similar examples in `AdaptiveToolbarHeaderPreference` which has a custom background style and color: https://screenshot.googleplex.com/5jhRme9fq464erh.png
Related details
- Hidehiko Abe
- Wenyu Fu