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

Changed full width button to a rounded button #308

Merged
merged 3 commits into from
Mar 8, 2019

Conversation

hosy
Copy link
Collaborator

@hosy hosy commented Mar 6, 2019

Description

  • changed full width button to a rounded button
  • fixed passcode view, error label and cancel button for smaller display sizes (iPhone SE)

Related Issue

#302

Motivation and Context

How Has This Been Tested?

  • open settings
  • enable / disable pass-lock
  • please test on iPhone SE, iPhone XS Max, iPad Pro 12,9

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

- fixed passcode view, error label and cancel button for smaller display sizes (iPhone SE)
@hosy hosy requested review from mneuwert and felix-schwarz March 6, 2019 18:19
@hosy hosy self-assigned this Mar 6, 2019
@hosy hosy added this to the 1.0.0 milestone Mar 6, 2019
@jesmrec jesmrec self-requested a review March 7, 2019 07:35
@jesmrec
Copy link
Contributor

jesmrec commented Mar 7, 2019

Button has the same size in iPhone and iPad. In iPhone, the button takes more or less 2/3 of the screen width, but in iPad it'd be about 1/3. Should the button have an scalable size?

iPad:

screen shot 2019-03-07 at 08 43 51

iPhone:

screen shot 2019-03-07 at 08 44 52

@michaelstingl
Copy link
Contributor

Please align button-width behaviour across the app. (compare with button on initial empty server screen)

Alternative: Add it as text-button to the top-right. (not beautiful, but works reliable)

@hosy
Copy link
Collaborator Author

hosy commented Mar 7, 2019

I tried it with a larger button on the iPad yesterday, but it looks not nice. Currently it has the same width as the buttons above.

@hosy
Copy link
Collaborator Author

hosy commented Mar 7, 2019

The current implementation looks not adapted to the iPad:
img_0035

@hosy
Copy link
Collaborator Author

hosy commented Mar 7, 2019

In my opinion it would looks better with a smaller button

@michaelstingl
Copy link
Contributor

In my opinion it would looks better with a smaller button

works for me

@hosy
Copy link
Collaborator Author

hosy commented Mar 7, 2019

Changed max width of buttons. Add Account and Cancel looks now equal and not to large on iPad:

iPad Pro 12.9 iPhone SE
simulator screen shot - ipad pro 12 9-inch 3rd generation - 2019-03-07 at 19 56 18 simulator screen shot - iphone se - 2019-03-07 at 19 56 02

@jesmrec
Copy link
Contributor

jesmrec commented Mar 8, 2019

ok, let's go for the approach. Approved.

@jesmrec jesmrec added the Approved by QA Approved by QA label Mar 8, 2019
@codecov
Copy link

codecov bot commented Mar 8, 2019

Codecov Report

Merging #308 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   29.84%   29.86%   +0.01%     
==========================================
  Files         228      228              
  Lines       15283    15284       +1     
==========================================
+ Hits         4561     4564       +3     
+ Misses      10722    10720       -2
Impacted Files Coverage Δ
...oud/Settings/Passcode/PasscodeViewController.swift 75.86% <100%> (+0.16%) ⬆️
...der Integration/FileProviderInterfaceManager.swift 91.57% <0%> (+2.1%) ⬆️

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 1a6ba33...0d17963. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Mar 8, 2019

Codecov Report

Merging #308 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   29.84%   29.86%   +0.01%     
==========================================
  Files         228      228              
  Lines       15283    15284       +1     
==========================================
+ Hits         4561     4564       +3     
+ Misses      10722    10720       -2
Impacted Files Coverage Δ
...oud/Settings/Passcode/PasscodeViewController.swift 75.86% <100%> (+0.16%) ⬆️
...der Integration/FileProviderInterfaceManager.swift 91.57% <0%> (+2.1%) ⬆️

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 1a6ba33...0d17963. Read the comment docs.

@hosy hosy merged commit 4053c53 into master Mar 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/passcode-cancel-button branch March 8, 2019 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved by QA Approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants