Skip to content

Add SegmentedControlIOS - #564

Closed
clayallsopp wants to merge 1 commit into
react:masterfrom
clayallsopp:feature/534-UISegmentedControl-support
Closed

Add SegmentedControlIOS#564
clayallsopp wants to merge 1 commit into
react:masterfrom
clayallsopp:feature/534-UISegmentedControl-support

Conversation

@clayallsopp

Copy link
Copy Markdown
Contributor

Fixes#534:

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

<SegmentedControlIOStintColor="#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

Comment threadReact/Views/RCTSegmentedControl.m Outdated

Copy link
Copy Markdown
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

a2 commented Apr 1, 2015

Copy link
Copy Markdown
Contributor

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

@a2a2 self-assigned this Apr 1, 2015
@ashwinb

Copy link
Copy Markdown
Contributor

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

a2 commented Apr 1, 2015

Copy link
Copy Markdown
Contributor

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

@clayallsopp

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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

@vjeux

vjeux commented Apr 3, 2015

Copy link
Copy Markdown
Contributor

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

@clayallsopp
clayallsoppforce-pushed the feature/534-UISegmentedControl-support branch from 06c2d95 to 49ddd7aCompareApril 3, 2015 17:55
@clayallsopp

Copy link
Copy Markdown
ContributorAuthor

@vjeux done, np

@facebook-github-botfacebook-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
Copy Markdown
Contributor

What's the status of this?

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 react#564
* Update reason-react-native/src/apis/AppState.md
Co-Authored-By: Max Thirouin <git@moox.io>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis 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

@clayallsopp@a2@ashwinb@vjeux@jozan@nicklockwood@facebook-github-bot