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

First iteration of a Star component #28

Merged
merged 1 commit into from
Jan 25, 2017
Merged

Conversation

gmarty
Copy link
Contributor

@gmarty gmarty commented Jan 24, 2017

This is only a first iteration of this component. Things missing include:

@arcturus
Copy link
Member

Looking gorgeos btw :)

Copy link
Member

@arcturus arcturus left a comment

Choose a reason for hiding this comment

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

Left some questions, basically on how we plan to use this.

So far looking amazing.

@@ -1,5 +1,3 @@
/* global require */
Copy link
Member

Choose a reason for hiding this comment

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

This change should be out of this pr, but we have seen others things in this repo right ? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make this change part of the eslint pr.

}

render() {
const img = this.state.value ?
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it easy to read something like:

const imgSource = this.state.value ? './assets/star.png' : './assets/star-border.png';
...
<Image source={require(imgSource)}/>```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it work this way? I tried a few days ago and it didn't work. require() has to be static.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct dynamic require doesn't work. I think it might be best to import the assets at the top of the file to make this clear.

const images = {
  empty: require('./assets/star-empty.png'),
  full: require('./assets/star-full.png'),
};

... later

render() {
  return <Image source={ this.state.value ? images.full : images.empty }/>
}

return (
<TouchableOpacity
style={styles.container}
onPress={this.toggle}>
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a callback property to inform the user of this component that the toggle happened therefore an action following that 'starred' can happened?

Or do you prefer the opposite logic, perform an action in the parent component and when that action is finished change the property used to show the start?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the component can communicate directly with Redux. This way there's no other wiring required than passing an itemId to this component.

Or... was it your question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I spoke with @arcturus about an idea for component design. We can have a dumb component that is just a <Star selected={true}/> that toggles between trueandfalse. But then we can wrap that in a ` component that wires the component into the app's Redux store.

This means that we could drop a <MagnetStar> anywhere in the app and it'll "just work".

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in Redux world they call <MagnetStar> style components: "container components". The don't have any visual properties, they just act as a data provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that makes sense.
I would only argue that the scenes are container components and we don't need to wrap this Star component inside another container.

@arcturus
Copy link
Member

Forgot to mention, once this work is done (with all iterations) we can close #14

@gmarty gmarty force-pushed the star branch 2 times, most recently from f4c34eb to 5d59b91 Compare January 24, 2017 17:00
@wilsonpage
Copy link
Collaborator

@gmarty can you check-in the source SVG too so that anyone can export different sizes/colours if need be?

@gmarty gmarty force-pushed the star branch 2 times, most recently from 8ce9bf5 to 1c49bba Compare January 25, 2017 10:44
@gmarty
Copy link
Contributor Author

gmarty commented Jan 25, 2017

I did the change requested.
I made it a dumb component with no logic and reuse the same signature than the <Switch/> component (value and onValueChange props).

We can implement the logic in a subsequent pull request.

@gmarty gmarty merged commit d44e0db into mozilla-magnet:master Jan 25, 2017
wilsonpage added a commit to wilsonpage/journey that referenced this pull request Jan 27, 2017
wilsonpage added a commit to wilsonpage/journey that referenced this pull request Jan 27, 2017
wilsonpage added a commit to wilsonpage/journey that referenced this pull request Jan 27, 2017
wilsonpage added a commit that referenced this pull request Jan 30, 2017
Reveal header on swipe down (closes #26)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants