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

[Toggle] Fix elementStyle prop not being used and tests to prove #7483

Closed
wants to merge 1 commit into from

Conversation

mkralla11
Copy link

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Toggle component elementStyle prop was not being used due to this and this.

The test I provided fails before my change, and passes after, respectively.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Jul 20, 2017
@oliviertassinari
Copy link
Member

I don't understand the use case, why not using the iconStyle property?

@oliviertassinari oliviertassinari added waiting for 👍 Waiting for upvotes component: switch This is the name of the generic UI component, not the React module! labels Jul 20, 2017
@oliviertassinari oliviertassinari changed the title Toggle Fix elementStyle prop not being used and tests to prove [Toggle] Fix elementStyle prop not being used and tests to prove Jul 20, 2017
@mkralla11
Copy link
Author

mkralla11 commented Jul 20, 2017

Looking at the source code, what is the point of elementStyle then? it simply gets disregarded as pointed out in the original links of the description of my PR. I see now that wrapStyles is being extended by iconStyles here and then used here, but why even have elementStyle in the source code - or even bother documenting it? I feel like references to elementStyle should be removed if they are not being used. Please let me know if you would rather me take that route (If so, I can update my test to utilize the iconStyle prop instead and remove references to elementStyle).

Let me know and thanks for you time on this. I was a little concerned when using elementStyle resulted in no altered styling.

@oliviertassinari
Copy link
Member

@mkralla11 I wouldn't worry that much about the v0.x version. We have been reimplementing it from scratch on the v1 version. From my perspective, it's legacy. As long as we don't introduce regression, I'm fine.

@mkralla11
Copy link
Author

I'm going to close this then. iconStyle will suffice like you said. If I (or someone else) has time, at least removing elementStyle from the documentation could ease the pain of anyone else trying to debug this. Thanks again!

@mkralla11 mkralla11 closed this Jul 20, 2017
@oliviertassinari
Copy link
Member

@mkralla11 Sounds great :)

@oliviertassinari oliviertassinari removed the waiting for 👍 Waiting for upvotes label Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: switch This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants