Adds accessoryType property to CollectionViewItem

Adds accessoryType property to CollectionViewItem

This makes CollectionViewItem properties match 1:1
with those of MDCCollectionViewCell

[Moe, 2017-04-12 13:33:23]

Description was changed from

==========
Adds accessoryType property to CollectionViewItem

This makes CollectionViewItem properties to matche 1:1
with those of MDCCollectionViewCell
==========

to

==========
Adds accessoryType property to CollectionViewItem

This makes CollectionViewItem properties matche 1:1
with those of MDCCollectionViewCell
==========

[Moe, 2017-04-12 13:33:23]

mahmadi@chromium.org changed reviewers:
+ lpromero@chromium.org, rohitrao@chromium.org

[Moe, 2017-04-12 13:35:31]

Hi Rohit and Louis, PTAL.

[rohitrao (ping after 24h), 2017-04-12 13:38:08]

Louis, this was an intentional omission, wasn't it?  I know we discussed this a
number of times and left it out of the base class because we didn't want to have
any form of inheritance in the Item hierarchy.

Maybe we should make CollectionViewItem a pure base class and add a new subclass
that maps to MDCCollectionViewCell?

[lpromero, 2017-04-12 13:58:12]

On 2017/04/12 13:38:08, rohitrao wrote:
> Louis, this was an intentional omission, wasn't it?  I know we discussed this
a
> number of times and left it out of the base class because we didn't want to
have
> any form of inheritance in the Item hierarchy.
> 
> Maybe we should make CollectionViewItem a pure base class and add a new
subclass
> that maps to MDCCollectionViewCell?

I know you resisted several times, but I keep thinking this property ought to be
on the base class, as it is also on the base MDCCell.
What you propose is going the other direction: move the accessibility identifier
and traits down to all the subclasses, so that indeed, we don't have any
inheritance or configuration.

I agree that we shouldn't have a hierarchy of items, but having this one seemed
useful and not detrimental.

On the other hand, I think see your point: where will it stop?
MDCCollectionViewCell has a dozen similar properties (accessory view), and
UICollectionViewCell a couple more.
My question is then: what makes accessibility properties special? Because they
might easily be overlooked?

[lpromero, 2017-04-14 12:05:56]

On 2017/04/12 13:58:12, lpromero wrote:
> On 2017/04/12 13:38:08, rohitrao wrote:
> > Louis, this was an intentional omission, wasn't it?  I know we discussed
this
> a
> > number of times and left it out of the base class because we didn't want to
> have
> > any form of inheritance in the Item hierarchy.
> > 
> > Maybe we should make CollectionViewItem a pure base class and add a new
> subclass
> > that maps to MDCCollectionViewCell?
> 
> I know you resisted several times, but I keep thinking this property ought to
be
> on the base class, as it is also on the base MDCCell.
> What you propose is going the other direction: move the accessibility
identifier
> and traits down to all the subclasses, so that indeed, we don't have any
> inheritance or configuration.
> 
> I agree that we shouldn't have a hierarchy of items, but having this one
seemed
> useful and not detrimental.
> 
> On the other hand, I think see your point: where will it stop?
> MDCCollectionViewCell has a dozen similar properties (accessory view), and
> UICollectionViewCell a couple more.
> My question is then: what makes accessibility properties special? Because they
> might easily be overlooked?

Kindly ping to Rohit.

[rohitrao (ping after 24h), 2017-04-14 12:17:39]

I can make up reasons for why a11y is different, but it isn't really.

Since we're talking about using the CollectionViewModel in other places in the
app, I think we should split off a new item type for MDCCollectionViewCell,
instead of using the base CollectionViewItem class.  I'll propose that
CollectionViewItem map to UICollectionViewCell now, instead.

This is somewhat related because it means that accessoryType would move off of
CollectionViewItem and onto the new MDC-specific item subclass.  It changes the
argument slightly because accessoryType will no longer apply to *all* cells, but
rather just *most* cells.  (A11y still applies to every cell, though.)

In the end, we make the rules, so we can do whatever we want.  Do you feel that
the no-hierarchy pattern is ingrained enough that we can break the rule in this
one place without opening the floodgates?  Is the simplification worth it?

[lpromero, 2017-04-14 12:25:49]

Description was changed from

==========
Adds accessoryType property to CollectionViewItem

This makes CollectionViewItem properties matche 1:1
with those of MDCCollectionViewCell
==========

to

==========
Adds accessoryType property to CollectionViewItem

This makes CollectionViewItem properties match 1:1
with those of MDCCollectionViewCell
==========

[lpromero, 2017-04-14 12:29:41]

On 2017/04/14 12:17:39, rohitrao wrote:
> I can make up reasons for why a11y is different, but it isn't really.
> 
> Since we're talking about using the CollectionViewModel in other places in the
> app, I think we should split off a new item type for MDCCollectionViewCell,
> instead of using the base CollectionViewItem class.  I'll propose that
> CollectionViewItem map to UICollectionViewCell now, instead.
> 
> This is somewhat related because it means that accessoryType would move off of
> CollectionViewItem and onto the new MDC-specific item subclass.  It changes
the
> argument slightly because accessoryType will no longer apply to *all* cells,
but
> rather just *most* cells.  (A11y still applies to every cell, though.)
> 
> In the end, we make the rules, so we can do whatever we want.  Do you feel
that
> the no-hierarchy pattern is ingrained enough that we can break the rule in
this
> one place without opening the floodgates?  Is the simplification worth it?

I like the idea of generalizing, as it distances us from MDC. The
CollectionViewModel and CollectionViewItem should be concepts related to
UICollectionView, not just MDCCollectionViewController.
We might have to cast more often with your approach, or cast to
UICollectionViewCell instead of MDCCollectionViewCell. We should make a
prototype CL.

[Moe, 2017-04-27 12:24:35]

Closing this as it was discussed in our meeting.