eggheadio / egghead-ui

egghead UI pieces as a package and app
https://styleguide.egghead.io
28 stars 6 forks source link

Update Tabs #151

Closed Judahmeek closed 7 years ago

Judahmeek commented 7 years ago

This PR does two things:

  1. Adds the ability to center Tabs.
  2. Adds className injection in order to allow developers to modify css to their use case (such as centering on mobile with flex-growth-1 but aligning left on desktop with horizontal padding).
  3. Removes flex-wrap
  4. Replaces ph4 with flex-grow-1 & tc for mobile and mobile only.
  5. Adds justify-content: inherit so that the TabsList can be centered with a wrapping justify-center div.

Current behavior of 6.0.0 Tabs at 375px width: Current behavior of 6.0.0 Tabs at 375px width

Behavior without flex-wrap due to padding: Behavior without flex-wrap due to padding

After fixes: Final result

Btw, I've had no luck testing this besides putting the same changes directly into html while working on egghead-rails' lesson layout.

I had

19:22:51 webpack-hmr.1           | ERROR in /Users/judahmeek/Apps/egghead-ui/lib/index.js
19:22:51 webpack-hmr.1           | Module build failed: ReferenceError: Unknown plugin "babel-plugin-react-transform" specified in "base" at 0, attempted to resolve relative to "/Users/judahmeek/Apps/egghead-ui/lib"

thrown when I tried using linking and got "could not resolve errors" when I tried to reference my branch in egghead-rails' package.json. So... merge with caution.

trevordmiller commented 7 years ago

@Judahmeek Thanks for the PR. A couple things before I approve/merge this: 1) Include screenshots of the changed behavior on mobile and desktop 2) Can centering not be achieved with a wrapping <div className='flex justify-center'>? I would prefer to provide explicit options for any tab style variations that @vojtaholik suggests rather than allowing passing any arbitrary classnames (see the Button component for an example of this). However, if the classname passing is required, please rename the props to tabListClassName and tabClassName to match the conventions in other component prop naming. 3) Update proptypes to match any prop changes: https://github.com/eggheadio/egghead-ui/blob/master/src/package/components/Tabs/index.js#L62-L67 4) Update the app resource example to match any changes: https://github.com/eggheadio/egghead-ui/blob/master/src/App/utils/resourcesByType/index.js#L678-L699

Thanks.

Judahmeek commented 7 years ago

@trevordmiller

  1. What changed behavior would you like me to display? With className injection, the possibilities are endless. Should I focus on the current behavior that inspired this PR (padding causing items to expand beyond container)?

  2. No. Flex effects only work on immediate children. So wrapping a container with justify-center would center the container, but it wouldn't have any effect on how the container justified its own children. You could set justify-content to inherit, but that wouldn't address the padding issue.

  3. & 4. I'll update this right now.

Judahmeek commented 7 years ago

Note to self: https://github.com/babel/babel/issues/3969 seems to be the error I had when linking. http://stackoverflow.com/questions/34574403/how-to-set-resolve-for-babel-loader-presets/ may be the solution.

trevordmiller commented 7 years ago

@Judahmeek

Per #1, I mean screenshots from the updated Tabs app output; what do the intended changes look like with the different container sizes? Your PR diff looks like it will break mobile/small container sizes. Something like this, as an example:

Usage (no changes):

image

Examples:

xsmall container: image

small container: image

medium+ containers: image

So we can see how your PR changes have effected the component use. Make sense?

trevordmiller commented 7 years ago

@Judahmeek If you'll just show what the changes look like I'll happily merge this in :) Thanks for your contribution.

Example of another PR: https://github.com/eggheadio/egghead-ui/pull/157

Judahmeek commented 7 years ago

@trevordmiller I did show the changes for mobile, which is what my concern was. screen shot 2017-05-20 at 2 38 30 am xsmall: mobile

screen shot 2017-05-20 at 2 39 22 am small: mobile

My changes do break containers that would depend on wrapping tabs based on my belief that wrapping tabs is undesirable in nearly every possible context (which is the primary reason I made this PR in the first place).

screen shot 2017-05-20 at 2 39 54 am xsmall: nonmobile

screen shot 2017-05-20 at 2 40 04 am medium: nonmobile

Judahmeek commented 7 years ago

@joelhooks The main change of this PR was removal of flex-wrap because I couldn't think of any situation where wrapping was actually the desired effect. I also changed the CSS for mobile to be fully responsive to make up for removal of flex-wrap. I assumed that any Tabs containers in non-mobile layouts would be designed so that wrapping is unnecessary.

In hindsight, I probably can keep the flex-wrap just for non-mobile layouts to maintain backwards compatibility: flex-wrap-ns