Skip to content
This repository was archived by the owner on Jan 14, 2025. It is now read-only.

refactor top-app-bar to make use of a type property? #522

Closed
mgr34 opened this issue Dec 14, 2018 · 0 comments
Closed

refactor top-app-bar to make use of a type property? #522

mgr34 opened this issue Dec 14, 2018 · 0 comments

Comments

@mgr34
Copy link
Contributor

mgr34 commented Dec 14, 2018

As I work on correcting the missing options of the <TopAppBarFixedAdjust/> (#518) 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 like:
       componentDidUpdate(prevProps) { 
          if (this.props.type !== prevProps.type) {
             this.initializeFoundation() 
           }
      }
  1. It is conceivable that a user may want a prominent dense TopAppBar on a desktop, and a fixed variant for a tablet, and a short variant for mobile. It may be a questionable design choice, but simplifying it with a single property sure looks attractive.
     <div>
        <TopAppBar
           fixed={state.display === 'tablet'} 
           prominent={state.display === 'desktop'} 
           dense={state.display === 'desktop}'
           short={state.display === 'mobile'}
         />
       <TopAppBarFixedAdjust 
            prominent={state.display === 'desktop'} 
            dense={state.display === 'desktop'} 
            fixed={state.display === 'tablet'
            short={state.display === 'short'}
        />
  </div>
  1. 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.

Originally posted by @mgr34 in https://github.com/material-components/material-components-web-react/issue_comments#issuecomment-447383027

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant