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

fix(badge): fix weird badge styling #587

Closed
wants to merge 1 commit into from

Conversation

t-howell
Copy link

Attempting to help with issue #581 - Sorry about the commit message format, it was my first try at angular formatting, I'll have to practice

@coveralls
Copy link

Coverage Status

Coverage remained the same at 34.504% when pulling be28dd5 on t-howell:t-howell into a9ae840 on gitpoint:master.

@andrewda andrewda changed the title fix badge.component fix(badge): fix weird badge styling Oct 26, 2017
@alejandronanez
Copy link
Member

@t-howell can you please upload a screenshot of the end result?

Thanks!

@housseindjirdeh
Copy link
Member

Thank you @t-howell <3

And no worries at all matey! We'll squash them anyway as we merge them in. Seconding @alejandronanez and could you please upload a before and after screenshot when you get the chance?

@jouderianjr
Copy link
Member

Hey, also the text color should be white, that was removed after the refactor to style components :/

Copy link
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Seconding what @jouderianjr mentioned, when you get the chance to change the text color to white this should be good to go 🙏

@@ -14,7 +14,7 @@ const BadgeContainer = styled.View`
flex: 1;
align-items: center;
justify-content: center;
border-radius: 18;
border-radius: 25;
Copy link
Member

Choose a reason for hiding this comment

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

normally, in order to have a circle, the border-radius should be equal to width & height.
could you provide a screenshot of the result?
don't hesitate if you need any assistance with this PR!

@machour
Copy link
Member

machour commented Nov 9, 2017

@t-howell I looked at the badge again this morning and I didn't notice the weird borders anymore. I guess this was somehow fixed by upgrading to latest react-native-version. 🤔

Could you rollback the border-radius change and make the text white so that we can merge this PR for the 1.4.0 release?

Let me know if you need any assistance. 🙏

@t-howell
Copy link
Author

t-howell commented Nov 9, 2017

Hey all, sorry for the late reply! I didn't expect to hear back on this one, honestly. This was my first go at contributing to a project - I'm a newb.

I couldn't get the environment running, so this was just my best guess. What do you mean by "rollback"? I can definitely make the text white.

@machour
Copy link
Member

machour commented Nov 10, 2017

@t-howell Thank you for your honesty and no worries mate! We all had to start somewhere 😉

To rollback something is to cancel the changes made (as opposed to roll out a new feature I guess)
So the idea would be to put back border-radius: 18 and just take care of making the text white.

We do however require you to provide a screenshot in your PR, so you will need to get the project running on your side. If you're planning to do more react/react-native development in the future, that could be a good occasion to get you started.

Do you want to go over our setup instructions and try to do that? We're available on gitter if you get stuck somewhere.

@machour
Copy link
Member

machour commented Nov 23, 2017

@t-howell are you still up for this? We need to fix this bug for the upcoming 1.4 release 🙏
I can take care of it if you don't have time right now, and will be more than happy to guide you through your next PR!

@t-howell
Copy link
Author

Would you take care of it? I'd love help figuring out this React thing when things are less busy! Thanks for your understanding.

@machour
Copy link
Member

machour commented Nov 25, 2017

@t-howell no worries at all mate 🙌
We'll be more than happy to help you get up and running when you have free time again, don't hesitate then!

Closing this PR, in favor of #648

@machour machour closed this Nov 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants