NotificationButtonMD should be an MdTextButton (?) |
|||
Issue descriptionIt seems like we should use MdTextButton instead of implementing a new class. Not sure if this gives us the /exact/ behavior that's spec'd, but even if not it seems likely that any slight differences of behavior or appearance are either unintentional or should be adjustable.
,
Feb 1 2018
NotificationInputReplyButtonMD likewise adds nothing that can't be set publicly. SetNormalImage and SetPlaceholderImage are particularly easy to consolidate because their bodies are nearly identical.
,
Feb 1 2018
I think LargeImageContainerView can go too, as well as ItemView. For context, the general consensus is that we'd rather reduce the number of View subclasses and instead rely on modularity, i.e. settable properties of the basic View classes. So if a View subclass is mainly just a constructor with a bunch of setters, it should probably be replaced by a factory function or inline creation. (+sky for this topic)
,
Feb 1 2018
+1 to what Evan says.
,
Jun 20 2018
I started this here[1], but I'm no longer sure if the original issue description is true (i.e. NotificationButtonMD should be an MdTextButton). The other comments about reducing one-off Views subclasses are still relevant and should be non-behavioral changes. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1103596 |
|||
►
Sign in to add a comment |
|||
Comment 1 by est...@chromium.org
, Feb 1 2018