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

HOC to give any component a badge #1604

Merged
merged 96 commits into from Dec 11, 2018

Conversation

Projects
None yet
3 participants
@janhesters
Copy link
Contributor

commented Nov 26, 2018

This HOC can give any component a badge. It works with minimal syntax:

withBadge(<SomeComponent />)(1)

while still allowing the user to customize anything he / she wants. The argument of the first functoin is . the component that should get the badge. The 1 at the end is the value within the badge. If you look at the code you'll see the rest of the API (like x - y offset of badge, programatically hide/show badge etc.).

By default it hides the badge automatically for empty string / number = 0.

iRoachie and others added some commits Nov 17, 2018

HOC to give any component a badge
This HOC can give any component a badge. It works with minimal syntax:
```
withBadge(<SomeComponent />)(1)
```
While still allowing the user to customize anything he / she wants.
@codecov

This comment has been minimized.

Copy link

commented Nov 26, 2018

Codecov Report

Merging #1604 into next will increase coverage by 0.05%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1604      +/-   ##
==========================================
+ Coverage   86.68%   86.74%   +0.05%     
==========================================
  Files          33       34       +1     
  Lines         616      626      +10     
  Branches       77       82       +5     
==========================================
+ Hits          534      543       +9     
- Misses         69       70       +1     
  Partials       13       13
Impacted Files Coverage Δ
src/header/Header.js 100% <ø> (ø) ⬆️
src/social/SocialIcon.js 90.9% <ø> (ø) ⬆️
src/buttons/Button.js 100% <ø> (ø) ⬆️
src/badge/withBadge.js 90% <90%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a965a81...7a9b0d0. Read the comment docs.

@xavier-villelegier xavier-villelegier changed the base branch from master to next Nov 27, 2018

@xavier-villelegier

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

Hey @janhesters, as a lover of HOC, I really like the idea ! 💯
Could you just update from the next branch ?

Here are my thoughts:

1) Your decorator should be used in this order:

const DecoratedComponent = withBadge(2)(BaseComponent)

It's what most decorators do.

2) It must propagate the props

If you do this:

<DecoratedComponent myProps={anything} />

You expect your BaseComponent to receive myProps.

3) It should be able to take a callback

In a lot of cases, you're just going to take the badge value from the props. Let's imagine an example where you have your notifications in your store and you use connect, you must be able to use it like that:

@connect(state => ({
  notifications: state.notifications,
}))
@withBadge(props => props.notifications.length)
export default class MyDecoratedIcon extends React.Component {
  render() {
    return (
      <Icon type="ionicon" name="md-cart" />
    );
  }
}

Proposal

Taking this into account, here is my proposal:

export const withBadge = (value) => (WrappedComponent) => {
  return class extends React.Component {
    render() {
      const badgeValue = typeof value === 'function' ? value(this.props) : value
      return (
        <React.Fragment>
          <WrappedComponent {...this.props} />
          {// Add your badge here with all the styling with `value={badgeValue}`}
        </React.Fragment>
      )
    }
  } 
}
@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

Could you just update from the next branch ?

@xavier-villelegier What exactly do you mean with this (I'm not that familiar with GitHub, took me forever to find out how to do PRs 😄)

@xavier-villelegier

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

Haha it's okay !
From your local fork, just run git pull upstream next, and then push the changes !

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@xavier-villelegier I get the message:

fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
@xavier-villelegier

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

@janhesters I updated your repo, I think you didn't have the upstream remote configured to the official RNE repo. Here is the doc about how to sync a fork with the original repo, might be useful in the future 😉 https://help.github.com/articles/syncing-a-fork/

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@xavier-villelegier BTW I love your changes and implemented them and added them to the PR (also I converted it from TS to JS).

The only small problem I have with these changes is that in my previous version I could do this:

static navigationOptions = {
  headerTitle: "List",
  headerRight: withBadge(<Icon />)(1)
};

Now, since this

static navigationOptions = {
  headerTitle: "List",
  headerRight: <withBadge(1)(Icon) />
};

is not possible, we would have to do something like this:

static navigationOptions = () => {
  const BadgedIcon = withBadge(1)(Icon);
  return {
    headerTitle: "List",
    headerRight: <BadgedIcon />
  };
};
@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@xavier-villelegier Thank you for the help with git!

janhesters added some commits Nov 27, 2018

Perfect styling
Now the badge looks smooth on iOS and always keeps it's round size by default.
@xavier-villelegier

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

What about

static navigationOptions = {
  headerTitle: "List",
  headerRight: withBadge(1)(<Icon />)
};

?

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@xavier-villelegier After your improvements

static navigationOptions = {
  headerTitle: "List",
  headerRight: withBadge(1)(<Icon />)
};

throws:

Functions are not valid as a React child. This may happen if you return a Component instead of <Component /> from render. Or maybe you meant to call this function rather than return it.

because in my original code I wrote {Component} instead of <Component /> in the HOC's code.

@xavier-villelegier

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2018

@janhesters Oops I meant:

static navigationOptions = {
  headerTitle: "List",
  headerRight: withBadge(1)(Icon)
};
@iRoachie

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

@xavier-villelegier with that way, how would you customize props for the Icon

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

@xavier-villelegier Yup, what @iRoachie said.

static navigationOptions = () => {
  const BadgedIcon = withBadge(1)(Icon);
  return {
    headerTitle: "List",
    headerRight: <BadgedIcon someProp={someValue} />
  };
};

Is the only way.

But to be honest, I'm already using the HOC improved by your Feedback in production, an assigning badges via:

@connect(state => ({
  notifications: state.notifications,
}))
@withBadge(props => props.notifications.length)
export default class MyDecoratedIcon extends React.Component {
  render() {
    return (
      <Icon type="ionicon" name="md-cart" />
    );
  }
}

Is super dope (as long as you only have on source of truth for you badges; otherwise you'll need several of these components for each enriched <Icon />). And this way you don't have to write much code in your headerLeft or headerRight.

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

@iRoachie @xavier-villelegier I worked on this a bit more, because as I said we are using this HOC right now in production, and I have a very elaborate TypeScript implementation of it. Let me know if you want to see it, or if you want to add this HOC as a .tsx instead of .js in the first place 😊

Change React.Fragmet to View
Because of styling reasons it's better to render an actual RN Element, instead of just the fragment (position absolute).
@iRoachie

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

@janhesters I think we'd like to move to TS sometime in the future but that may be a bit difficult right now so let's stick with js.

Show resolved Hide resolved src/badge/WithBadge.js Outdated

janhesters and others added some commits Dec 6, 2018

Update src/badge/withBadge.js
Co-Authored-By: janhesters <31096420+janhesters@users.noreply.github.com>
Update src/badge/withBadge.js
Co-Authored-By: janhesters <31096420+janhesters@users.noreply.github.com>
@iRoachie

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

What did you do just now? I think you just added some other changes to files

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

Yeah, god damnit, somehow some files formatting changed (even though I didn't touch them 😦). I see if I can revert that somehow ...

Other than that I added the suggested containerStyle prop.

@iRoachie

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Nevermind i see those are from the precommit hook.

@iRoachie

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Yea what happen is that we added the precommit linting and formatting after that code was added :D so by you running it just now it updated.

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

So they are okay?

@iRoachie

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

It's kinda strange though, it looks like it's doing different kind of formatting than it's supposed to :| lol. Don't worry about it

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

So @iRoachie what defaults should we decide to use? And should we implement an if statement to consider the MiniBadge?

Show resolved Hide resolved docs/badge.md

@iRoachie iRoachie merged commit 59845eb into react-native-training:next Dec 11, 2018

3 checks passed

codecov/patch 90% of diff hit (target 86.68%)
Details
codecov/project 86.74% (+0.05%) compared to a965a81
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@iRoachie

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

Thanks for all the hard work @janhesters 💯 If you've got some extra time on your hand and passionate about building react-native-elements; we're always looking for new maintainers to join the team! Let me know.

Also @xavier-villelegier will probably hit you up about open-collective.

Thanks again 🎉

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

Thank you for teaching me so much!

I'm getting kinda hooked and passionate about RNE 😄 . It's the best UI library for RN imo! 🔥

I would love to help here and there (if I find the time). I've looked into the issues for the milestones, and tried to fix some of those without PR, but couldn't fix any. But I will keep an eye out and try to help improve this library going forward.

@xavier-villelegier

This comment has been minimized.

Copy link
Collaborator

commented Dec 14, 2018

@janhesters As you may (or not) know, we have an Open Collective, and we give the money to people that helps to contribute to React Native Element. You did a lot of good work recently on RNE, your neat PRs and your issues helped us a lot to polish and improve the v1.0.0 🔥 + you were really comprehensive during the review 🥇

Sooo, could you please submit a $60 expense on Open Collective ? Here is how to do it:

  1. Go on Open Collective

  2. Click "Submit Expense"
    Description: 1.0.0 PRs and issues
    Category: Engineering

  3. For the receipt, use this template

Feel free to reach me if you need more details 👌

@janhesters

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

@xavier-villelegier Thank you guys, but I would like to politely decline. To me the rewards are the learnings 📈.

I would rather go back to what @iRoachie said and help out here and there with maintaining and enhancing RNE if I find the time and a proper challenge 🚀

@xavier-villelegier

This comment has been minimized.

Copy link
Collaborator

commented Dec 14, 2018

Your call man ! Feel free to reach us on Slack if you change your mind. Remember that the Open Collective is only for contributors !

Glad you enjoyed so much your work on RNE though, we're always looking for motivated people for open sourcing 🙋‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.