Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tabs] Add a TabIndicatorProps property #11254

Merged
merged 8 commits into from May 6, 2018

Conversation

adeelibr
Copy link
Contributor

@adeelibr adeelibr commented May 6, 2018

this is an initial commit change, I have just added a prop validation so that indicator color can accept a string also with secondary and primary.

Closes #11085

@adeelibr adeelibr changed the title added a proptype for any string for indicator color Adding the ability so that in tabs indicatorColor it can be a hex, rgba code or simply string primary/secondary May 6, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented May 6, 2018

@adeelibr Thanks for giving a shot at this issue :). I think that there is a misunderstanding. The feature you want to introduce in this pull-request was removed in #10999.
In the issue, #11085, I was proposing to introduce a TabIndicatorProps property to the Tabs component so people can mess around with it. The dynamic inline-style is one use-case. What do you think about this plan?

@adeelibr
Copy link
Contributor Author

adeelibr commented May 6, 2018

@oliviertassinari Oh okay that makes sense, my bad! let me make some changes to my PR.

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label May 6, 2018
@adeelibr
Copy link
Contributor Author

adeelibr commented May 6, 2018

I have added a new prop in the Tabs component called tabIndicatorProps which I pass down to TabIndicator component. This prop is basically an object that takes in style params to customize the TabIndicator Component

Examples of using it

<Tabs 
    value={value} 
    onChange={this.handleChange}
   tabIndicatorProps={{
        height: 10, // change height of the bottom indicator
        backgroundColor: 'green',
       // etc, .. 
   }}
>
    <Tab label="Item One" />
    <Tab label="Item Two" />
    <Tab label="Item Three" href="#basic-tabs" />
/Tabs>

@oliviertassinari oliviertassinari self-assigned this May 6, 2018
@oliviertassinari oliviertassinari changed the title Adding the ability so that in tabs indicatorColor it can be a hex, rgba code or simply string primary/secondary [Tabs] Add a TabIndicatorProps property May 6, 2018
@oliviertassinari oliviertassinari added new feature New feature or request component: tabs This is the name of the generic UI component, not the React module! and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 6, 2018
@oliviertassinari oliviertassinari merged commit 7efbe32 into mui:v1-beta May 6, 2018
@oliviertassinari
Copy link
Member

@adeelibr It's a great first pull request on Material-UI 👌🏻. Thank you for giving it a shot!

@adeelibr
Copy link
Contributor Author

adeelibr commented May 7, 2018

@oliviertassinari I am super excited, this was my first open source PR. Thank you for helping me out with this PR. I intend to be more active in open source. Is there a possibility that if I stay active in contributing here that I can become an official member of the team? (Although I know I am new at this, but the experience has just been awesome, The learning and growth working is huge)

Is there a road map for this, also can I work in improving docs in material ui as well? I mean I can start working on that as well.

@adeelibr
Copy link
Contributor Author

adeelibr commented May 7, 2018

I reviewed the changes that you made to the PR, can you be so kind enough to tell me how is that you are passing TabIndicatorProps without using the style prop in the span element in TabIndicator component.

Also how is it that you are using type script in material ui? Can you share some resources please so that I can study on it, that way I can contribute better next time.

Thank you.

@adeelibr
Copy link
Contributor Author

adeelibr commented May 7, 2018

Also I tried updating my local forked branch to get it synced with the latest material-ui with the following commands

git remote add upstream git@github.com:mui-org/material-ui.git
git checkout v1-beta
git pull upstream v1-beta

This gives me an error

Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 7, 2018

This project works through meritocracy. The more value one contributor brings to the project, the more responsibilities he gets. Yes, people who joined the core team have been people undertaking important improvements, people deeply caring about the future of the project, people sharing the same vision for the project.

There is no fixed roadmap. There are the opened issues. There is the closed issue history. There are issues that bring context. There are the problems you face in the production projects you are working on. There are the documentation vision, api, roadmap, governance etc. There are milestones where we try to share a sense of what's important.

But more importantly, contributing to open source test people resilience. Most people gives up. You won't learn from "me", you will learn from others.

@adeelibr adeelibr deleted the tabs-custom-indicator-color branch May 19, 2018 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tabs This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants