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

Add SegmentedControlIOS #564

Conversation

clayallsopp
Copy link
Contributor

Fixes #534:

screen shot 2015-03-31 at 7 52 10 pm

<SegmentedControlIOS
  tintColor="#ff0000"
  values={["One", "Two", "Three", "Four"]}
  selectedValue={"One"}
  momentary={false}
  disabled={false}
  onValueChange={ (value) => console.log(value) } />

This only supports string-based segments, not images. Also doesn't support full customization (no separator images etc); I figure this is a good MVP to lock-down a basic API

I also included a snapshot test case, but the images keep coming out funky. When I look at the sim, I see that the text labels show up for the selected segment, but the snapshot keeps coming out with no text on those segments. I tried forcing a delay, but same result. Is that explainable?

Obviously happy to change anything about the API, code-style nitpicks, etc

#import "UIView+React.h"
#import "RCTEventDispatcher.h"


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Remove extra line.

@a2
Copy link
Contributor

a2 commented Apr 1, 2015

Clay, this looks great. I only have a few nit requests but if you can make those edits, we can merge this straightaway. 😃

@a2 a2 self-assigned this Apr 1, 2015
@ashwinb
Copy link
Contributor

ashwinb commented Apr 1, 2015

Hm, we should ideally just use selectedIndex in the backing implementation, and expose both selectedValue and selectedIndex to the user. Or just avoid selectedValue completely. It just seems like a bad idea that typos there will result in undefined behavior.

@a2
Copy link
Contributor

a2 commented Apr 1, 2015

@ashwinb Good idea. I like that better. The user should be able to set the selectedIndex and values separately.

@clayallsopp
Copy link
Contributor Author

@a2 @ashwinb I went back and forth on this; I figured that, on the JS side, people would be doing the index -> value conversion on every callback to make their logic not dependent on ordering, so we might as well do it for them. I.e.:

_onSegmentValueChanged : (value) =>
  // this is resilient to ordering changes
  switch(value) {
    case "Profile":
      break;
  }

vs

_onSegmentValueChanged : (selectedIndex) =>
  value = this.props.segments[selectedIndex]
  // now it is resilient to order changes
  switch() {
  }

maybe a compromise is to 1) allow selectedValue and selectedSegmentIndex as props, and prefer selectedIndex if given 2) pass both selectedSegmentIndex and selectedValue in the callback event, for raw onChange listeners, but keep onValueChange returning the segment value for the case above

but my opinion isn't terribly strong, so will defer to your judgement

@ashwinb
Copy link
Contributor

ashwinb commented Apr 1, 2015

Yes, passing both (index, value) seems like a good idea in the callback, although I am fine even with just (value) as you have done. However, what I am opposed to is the selectedValue prop -- that is an unnecessary landmine of typos (unless, of course, you make it an enum, etc. if you are using something like flowtype). We should just avoid it if possible.

@clayallsopp
Copy link
Contributor Author

I've updated the PR; check out the API examples

@vjeux
Copy link
Contributor

vjeux commented Apr 3, 2015

It doesn't patch cleanly anymore, can you rebase? Sorry about this

@clayallsopp clayallsopp force-pushed the feature/534-UISegmentedControl-support branch from 06c2d95 to 49ddd7a Compare April 3, 2015 17:55
@clayallsopp
Copy link
Contributor Author

@vjeux done, np

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 7, 2015
@jozan
Copy link
Contributor

jozan commented Apr 27, 2015

What's the status of this?

RCT_EXPORT_VIEW_PROPERTY(tintColor, UIColor);
RCT_EXPORT_VIEW_PROPERTY(momentary, BOOL);
RCT_EXPORT_VIEW_PROPERTY(enabled, BOOL);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs RCT_EXPORT_MODULE(); to work in current version.

@nicklockwood
Copy link
Contributor

Hey, sorry about the radio silence on this. The internal diff sort of got lost in transmission for a while, however the good news is that this has now been merged and will ship with the next release.

@clayallsopp
Copy link
Contributor Author

Lovely, thanks!

jfrolich pushed a commit to jfrolich/react-native that referenced this pull request Apr 22, 2020
* Docs for Appstate

I lost whatever pull request that was so I have recreated here in response to issue facebook#564

* Update reason-react-native/src/apis/AppState.md

Co-Authored-By: Max Thirouin <git@moox.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UISegmentedControl support
7 participants