feat(top-app-bar): is missing dense variant

[mgr34]

The <TopAppBar /> should contain a dense variant [0].

Should a fix for this also include a dense (and prominent) props for the <TopAppBarFixedAdjust/> component (and should I open an additional issue?)

I think I could have a PR for this later today.

[0] https://github.com/material-components/material-components-web/blob/master/packages/mdc-top-app-bar/README.md#dense

[moog16]

That'd be great!

What do you mean (and prominent). We already have the styles for the prominent class.

[mgr34]

I neglected the `'s when writing <TopAppBarFixedAdjust/> which hid it from my comment. At present fixed adjust does not appear to add mdc-top-app-bar--dense-prominent-fixed-adjust , mdc-top-app-bar--prominent-fixed-adjust , mdc-top-app-bar--short-fixed-adjust, or mdc-top-app-bar--dense-fixed-adjust

[moog16]

ahhh - yes you're totally correct. Can you do 2 separate PRs? You can reference this single issue. 👍

[mgr34]

@moog16 yes I can do 2 PR's. I'll also need to update the documentation/README.md with each PR. Or is the preference for documentation updates to be in a 3rd PR (seems excessive)?

[moog16]

Just keep the REadme updates in the same PR as the changes. I just want the PRs to be separate since they are logically different fixes.

[mgr34]

As I work on correcting the missing options of the <TopAppBarFixedAdjust/> I cannot help but wonder if <TopAppBar> is too verbose. In part because a lot of the options need to be repeated for the fixed adjust and type checking. Why not simplify the whole thing and have a type property.

Take for example the following

    <div className='top-app-bar-container'>
      <TopAppBar  prominent  dense  title='App Title' />
      <TopAppBarFixedAdjust prominent dense >
          <p>blah, blah, blah</p>
       </TopAppBarFixedAdjust>
    </div>     

could be rewritten to

    <div className='top-app-bar-container'>
      <TopAppBar title='App Title' type='prominentDense'/>
      <TopAppBarFixedAdjust type='prominentDense' >
          <p>blah, blah, blah</p>
       </TopAppBarFixedAdjust>
    </div>     

That may not look like anything too significant but consider the following.

1. there is nothing stopping someone from writing <TopAppBar prominent short fixed/> A type property could enforce a single type from a constrained list.
2. this would simplify my recent PR fix(top-app-bar): foundation should be destroyed and reinitialized on variant change #519 to something

       componentDidUpdate(prevProps) { 
          if (this.props.type !== prevProps.type) {
             this.initializeFoundation() 
           }
      }

3. It is conceivable that a user may want a prominent dense TopAppBar on a desktop, and a fixed variant for a tablet, and a standard variant for mobile. Simplifying it with a single property sure looks attractive.

     <div>
        <TopAppBar
           fixed={state.display === 'tablet'} 
           prominent={state.display === 'desktop'} 
           dense={state.display === 'desktop}'
         />
       <TopAppBarFixedAdjust 
            prominent={state.display === 'desktop'} 
            dense={state.display === 'desktop'} 
        />
  </div>

4. Especially when you consider <TopAppBar> also has a lot of long properties already with navigationIcon, and actionItems.

presuming the adoption of type property proves successful the question I have is such a property camelCased? If it were to be delimited with a hyphen it could be useful for class name generation?

    const BASE_CLASS = 'mdc-top-app-bar';
    const className = `${BASE_CLASS}--${type}`;

Of course a regex that does a conversion could also be whipped up without issue.

(Sorry this got a bit long winded. Would you prefer I split this off into another issue?)

[moog16]

@mgr34 I don't think you would ever have more than 2 different types. Technically prominent, short,and fixed are not supposed to work together (at least it hasn't been tested). We just don't restrict the user from being able to do that. Also by combining the different types into one string, we'd now have to have enums for each combination we do support. I think the way we have it is more extensible.

I see where you are coming from in your #2, but again there are only 4 or 5 different props we'll need to check.

[mgr34]

Hi @moog16, I did wind up making my last comment a separate issue (#522). It includes a little bit more information.

Technically prominent, short,and fixed are not supposed to work together (at least it hasn't been tested).

This is part of my point. There would be no warning, whereas if it was confined to a single property this issue would not exist.

Also by combining the different types into one string, we'd now have to have enums for each combination we do support.

Not necessarily problematic. It maybe as simple as:

static propTypes = {
    type: PropTypes.oneOf([
      'standard',
      'short',
      'short-collapsed',
      'fixed',
      'dense',
      'prominent',
      'prominent-dense',
      'disconnected' // w/o Foundation for added extensiblity       
    ]).isRequired,
  }
}

(don't mind disconnected. I used that in an implementation of TopAppBar I worked on back in October.)

but again there are only 4 or 5 different props we'll need to check.
Yes, but wouldn't they potentially be checked twice? Once in <TopAppBar/> and <TopAppBarFixedAdjust/>?

Anyhow, I sense resistance to this proposal so I will back off. Just an idea I had that I thought was worth some discussion. Thanks for considering the issues :)

[moog16]

This pattern is littered in the all components (ie button, list, etc). I think it would have to be a system wide change if we were to go down that road. I just don't think that we gain that much from it. I do like the idea of having enums. However we are in the middle of a typescript rewrite, which would complicate things. That change would have to wait until next year. But we can revisit the issue then! Thanks :)

[moog16]

#524 merged. Thanks again @mgr34 !