Skip to content

fix(Button): Make the padding consistent #1505

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.

Sign up for GitHub

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

Jump to bottom
Merged
iRoachie merged 4 commits into from
Oct 30, 2018
Merged

fix(Button): Make the padding consistent #1505

iRoachie merged 4 commits into from
Oct 30, 2018

Conversation

@haruelrovix
Copy link
Contributor

@haruelrovix haruelrovix commented Oct 23, 2018 •
edited
Loading

They have the same height now.

Before After
screenshot_2018-10-23-22-09-11-543_com a screenshot_2018-10-23-22-39-29-935_com a

Fix #1427 .

@haruelrovix
Copy link
Contributor Author

haruelrovix commented Oct 23, 2018

More details:

- Reset
screenshot_2018-10-23-22-38-51-399_com a screenshot_2018-10-23-22-38-59-590_com a
+ Add
screenshot_2018-10-23-22-39-06-107_com a screenshot_2018-10-23-22-39-17-334_com a

@codecov
Copy link

codecov bot commented Oct 23, 2018 •
edited
Loading

Codecov Report

Merging #1505 into next will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             next    #1505   +/-   ##
=======================================
  Coverage   75.32%   75.32%           
=======================================
  Files          33       33           
  Lines         608      608           
  Branches       85       85           
=======================================
  Hits          458      458           
  Misses        129      129           
  Partials       21       21
Impacted Files Coverage Δ
src/buttons/Button.js 100% <ø> (ø) ⬆️

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 4b512c3...cd86eb3. Read the comment docs.

xavier-villelegier
xavier-villelegier reviewed Oct 24, 2018
View reviewed changes
Copy link
Collaborator

@xavier-villelegier xavier-villelegier left a comment

Choose a reason for hiding this comment

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

Thanks for helping us to polish these kind of things ! 👏
Actually, wouldn't it be easier to only have padding on the button style, to avoid adding multiple paddings everywhere ?

@haruelrovix
Copy link
Contributor Author

haruelrovix commented Oct 25, 2018

@xavier-villelegier achieved the same looks via this, 1c413dd.

Preview:

xavier-villelegier
xavier-villelegier reviewed Oct 25, 2018
View reviewed changes
src/buttons/Button.js
textAlign: 'center',
padding: 8,
paddingTop: 2,
paddingBottom: 1,
Copy link
Collaborator

@xavier-villelegier xavier-villelegier Oct 25, 2018

Choose a reason for hiding this comment

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

Was just wondering why did you put padding on the title ?

Copy link
Contributor Author

@haruelrovix haruelrovix Oct 25, 2018

Choose a reason for hiding this comment

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

@xavier-villelegier it was there, with value 8 🤔

If I removed it completely:

  title: {
    backgroundColor: 'transparent',
    color: 'white',
    fontSize: 16,
    textAlign: 'center',
    ...Platform.select({
      ios: {
        fontSize: 18,
      },
      android: {
        fontFamily: 'sans-serif-medium',
      },
    }),
  },

button with only title (without icon) won't have the same height.

Copy link
Collaborator

@xavier-villelegier xavier-villelegier Oct 25, 2018

Choose a reason for hiding this comment

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

Okay the goal is to have all button of the same height. Looks good to me then.

@iRoachie
Copy link
Collaborator

iRoachie commented Oct 30, 2018 •
edited
Loading

Hey @haruelrovix can you rebase your PR from the next branch? It has a ton of additional commits

@haruelrovix
Copy link
Contributor Author

haruelrovix commented Oct 30, 2018

@iRoachie could you please help on rebasing?

If I'm not mistaken, here is what I need to do:

git checkout next
git pull upstream next
git checkout 1427-consistent-padding-for-button
git rebase next

But on step 2, git pull upstream next, I got conflict message:

$ git pull upstream next
From https://github.com/react-native-training/react-native-elements
 * branch            next       -> FETCH_HEAD
Removing website/versioned_sidebars/version-1.0.0-beta6-sidebars.json
Auto-merging src/index.d.ts
Auto-merging docs/getting_started.md
CONFLICT (content): Merge conflict in docs/getting_started.md
Automatic merge failed; fix conflicts and then commit the result.

Should I resolve the conflict and then continue to step 3?

Here is my upstream:

$ git remote -v
origin  https://github.com/haruelrovix/react-native-elements.git (fetch)
origin  https://github.com/haruelrovix/react-native-elements.git (push)
upstream        https://github.com/react-native-training/react-native-elements.git (fetch)
upstream        https://github.com/react-native-training/react-native-elements.git (push)

@haruelrovix haruelrovix force-pushed the 1427-consistent-padding-for-button branch from dc5180c to cd86eb3 Compare October 30, 2018 09:10
@haruelrovix
Copy link
Contributor Author

haruelrovix commented Oct 30, 2018

@iRoachie please ignore my previous comment. rebase is done 🤝

@iRoachie iRoachie merged commit 5508d92 into react-native-elements:next Oct 30, 2018
@iRoachie
Copy link
Collaborator

iRoachie commented Oct 30, 2018

Thank youuuuu 💯

@haruelrovix haruelrovix deleted the 1427-consistent-padding-for-button branch October 31, 2018 01:47
iRoachie added a commit that referenced this pull request Oct 31, 2018
@iRoachie
fix: Make button height consistent when using loading
18611c3 
Continuation of #1505 which didn't account for the loading prop
nurdiansyah added a commit to deboxlibrary/react-native-elements that referenced this pull request Nov 6, 2018
@nurdiansyah
Merge commit '0126436f5f0f37e0340b0c1da32bdea5605cb6df'
6a921b4 
* commit '0126436f5f0f37e0340b0c1da32bdea5605cb6df': (43 commits)
  feat(Card): Remove flexDirection prop
  feat(Card): Remove fontFamily prop
  test: Add tests for withTheme
  test: Fix searcbar error being logged to console
  fix: Hoist statics for withTheme hoc (react-native-elements#1554)
  fix: Use library for calculating statusBar height (react-native-elements#1553)
  ref: Badge component (react-native-elements#1545)
  Set default prop for type (react-native-elements#1546)
  docs: Fix paragraphs links in docs (react-native-elements#1536)
  docs(website): Make image preview section reusable
  feat: Add button types (react-native-elements#1540)
  fix: Rounded buttons on android now work correctly (react-native-elements#1538)
  ci: Run travis against node stable
  fix: Make button height consistent when using loading
  ci: Update husky and lint-staged
  feat: Add AntDesign to list of available icon sets (react-native-elements#1529)
  docs(website): Add central place to show supported icon sets (react-native-elements#1532)
  feat(Input): Allow label to be a React element (react-native-elements#1531)
  ci: Remove expo ci from PRs (react-native-elements#1528)
  fix(Button): Make the padding consistent (react-native-elements#1505)
  ...
@iRoachie iRoachie mentioned this pull request Jan 31, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xavier-villelegier xavier-villelegier xavier-villelegier approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@haruelrovix @iRoachie @xavier-villelegier