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 part #44: Full UI profile pin/password screen. #597

Merged
merged 16 commits into from
Feb 5, 2020

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Jan 9, 2020

Explanation

This PR fixes UI for profile pin/ password screen.

Mock

https://xd.adobe.com/view/0687f00c-695b-437a-79a6-688e7f4f7552-70b6/screen/d59621be-cceb-4686-9442-68dd6b3e2ee2/PC-AP-Filled-

https://xd.adobe.com/view/0687f00c-695b-437a-79a6-688e7f4f7552-70b6/screen/dc5dd024-cff4-4419-80a5-24147371bb72/PC-NP-Empty-Error-

Accessibility Scanner

  • Passes Accessibility scanner test

Screenshot

Screenshot_20200109-152224
Screenshot_20200109-152233

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

PTAL

app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Jan 9, 2020
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

PTAL

app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, marked some conversations as unresolved.

@rt4914 rt4914 removed their assignment Jan 9, 2020
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

Please confirm regarding the border with @mschanteltc because in mock there are not borders to the pinview. Also please use proper eye icons as per mock.

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Jan 9, 2020
@veena14cs
Copy link
Contributor Author

Please confirm regarding the border with @mschanteltc because in mock there are not borders to the pinview. Also please use proper eye icons as per mock.

@mschanteltc can you please provide me icons to show and hide password.

@mschanteltc
Copy link

Please confirm regarding the border with @mschanteltc because in mock there are not borders to the pinview. Also please use proper eye icons as per mock.

@mschanteltc can you please provide me icons to show and hide password.

Yes, it's available in the Android Image drive under the folder "Profile Chooser."

@mschanteltc
Copy link

Also there is no border, but there is a #000000 shadow at 30% opacity with a blur of 3.

@veena14cs
Copy link
Contributor Author

Please confirm regarding the border with @mschanteltc because in mock there are not borders to the pinview. Also please use proper eye icons as per mock.

@mschanteltc can you please provide me icons to show and hide password.

Yes, it's available in the Android Image drive under the folder "Profile Chooser."

There is slight confusion in the show/hide icon along with the text. As per the mock hide_eye_icon is been used with show text. But in our implementation it is other way. Which one is correct in this.
Screenshot_20200110-111234
Screenshot_20200110-111244

@mschanteltc
Copy link

Please confirm regarding the border with @mschanteltc because in mock there are not borders to the pinview. Also please use proper eye icons as per mock.

@mschanteltc can you please provide me icons to show and hide password.

Yes, it's available in the Android Image drive under the folder "Profile Chooser."

There is slight confusion in the show/hide icon along with the text. As per the mock hide_eye_icon is been used with show text. But in our implementation it is other way. Which one is correct in this.
Screenshot_20200110-111234
Screenshot_20200110-111244

I agree with your implementation. I updated the mocks to reflect this.

@veena14cs
Copy link
Contributor Author

Please confirm regarding the border with @mschanteltc because in mock there are not borders to the pinview. Also please use proper eye icons as per mock.

@mschanteltc can you please provide me icons to show and hide password.

Yes, it's available in the Android Image drive under the folder "Profile Chooser."

There is slight confusion in the show/hide icon along with the text. As per the mock hide_eye_icon is been used with show text. But in our implementation it is other way. Which one is correct in this.
Screenshot_20200110-111234
Screenshot_20200110-111244

I agree with your implementation. I updated the mocks to reflect this.

Thanks for clarification.

Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

One change in dimens file, else looks great.

app/src/main/res/layout/pin_password_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/values-xhdpi/dimens.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM.

@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Jan 20, 2020
@veena14cs veena14cs requested a review from BenHenning January 24, 2020 09:41
@veena14cs veena14cs assigned BenHenning and unassigned veena14cs Jan 24, 2020
@rt4914
Copy link
Contributor

rt4914 commented Feb 4, 2020

@veena14cs Lets not block this PR anymore just because of two different dimens.xml file. I will keep track of this and discuss this with @BenHenning and meanwhile you can merge this PR.

@rt4914 rt4914 assigned veena14cs and unassigned BenHenning Feb 4, 2020
@veena14cs
Copy link
Contributor Author

@veena14cs Lets not block this PR anymore just because of two different dimens.xml file. I will keep track of this and discuss this with @BenHenning and meanwhile you can merge this PR.

Okay will merge this PR thanks.

@veena14cs veena14cs merged commit 38a6a90 into develop Feb 5, 2020
@veena14cs veena14cs deleted the hifi-pin-password branch February 5, 2020 04:26
@BenHenning
Copy link
Member

Having multiple dimension files seems sensible.

@veena14cs
Copy link
Contributor Author

Having multiple dimension files seems sensible.

Got it thanks.

PrarabdhGarg pushed a commit to PrarabdhGarg/oppia-android that referenced this pull request Feb 12, 2020
* done ui corrections

* fixed issues.

* Update pin_password_activity.xml

* updated color

* updated icon

* updated pinview shadow

* updated icon color

* changed shadow

* updated mock color

* fix

* Update pin_password_activity.xml

* updated nit change.

* Update pin_password_activity.xml

* removed fixed width

* fixed dimen file changes

* Update dimens.xml
rt4914 pushed a commit that referenced this pull request Feb 13, 2020
* Fix #572: Keyboard visible by default in Admin Pin (#573)

* By default keyboard visibility issue in Admin Pin is resolved

* Created a test case to check about Keyboard visibility by default in Admin Pin.

* Test is modified with the suggested changes

* Fix #577:  Display profile name on navigation drawer. (#578)

* display profile name in navigation drawer

* Updated code implementation

* adding tests

* added testcases

* Update NavigationDrawerFragmentPresenter.kt

* updated nit changes.

* Fix part #44: Full UI profile pin/password screen. (#597)

* done ui corrections

* fixed issues.

* Update pin_password_activity.xml

* updated color

* updated icon

* updated pinview shadow

* updated icon color

* changed shadow

* updated mock color

* fix

* Update pin_password_activity.xml

* updated nit change.

* Update pin_password_activity.xml

* removed fixed width

* fixed dimen file changes

* Update dimens.xml

* Fix part #632: Replace current recyclerview implementation with BindableAdapter usage. (#641)

* working on topic practice.

* reverted

* updated implementation

* Update ContinuePlayingFragmentPresenter.kt

* Update ContinuePlayingFragmentPresenter.kt

* Update ContinuePlayingItemViewModel.kt

* Delete OngoingListAdapter.kt

* nit change

* Update ContinuePlayViewModel.kt

* fixed nit

* Add UI and finctionality for Switch Profile

* Add UI Tests for switch profile option in navigation drawer

* Add UI Tests for switch profile option in navigation drawer

* Minor styling changes

Co-authored-by: Akash Koradia <[email protected]>
Co-authored-by: Veena <[email protected]>
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.

5 participants