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.

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

Fix #180: Lowfi and Highfi Hint solution landscape #957

Merged
merged 19 commits into from
Apr 17, 2020
Merged

Fix #180: Lowfi and Highfi Hint solution landscape #957

merged 19 commits into from
Apr 17, 2020

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Apr 9, 2020 •
edited
Loading

@veena14cs veena14cs changed the title Fix :Lowfi Hint solution landscape Fix #180:Lowfi Hint solution landscape Apr 9, 2020
@veena14cs veena14cs self-assigned this Apr 9, 2020
@veena14cs veena14cs changed the title Fix #180:Lowfi Hint solution landscape Fix #180: Lowfi and Highfi Hint solution landscape Apr 13, 2020
rt4914
rt4914 suggested changes Apr 14, 2020
View reviewed changes
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/java/org/oppia/app/player/state/hintsandsolution/HintsViewModel.kt Outdated Show resolved Hide resolved
app/src/main/res/layout-land/hints_and_solution_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/hints_and_solution_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/hints_and_solution_fragment.xml Show resolved Hide resolved
app/src/main/res/layout-land/hints_summary.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/hints_summary.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/solution_summary.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/solution_summary.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/solution_summary.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/solution_summary.xml Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Apr 14, 2020
@nikitamarysolomanpvt
Copy link
Contributor

@veena14cs Please assign me back once you completes suggestions of @rt4914 that is discussed.

@veena14cs veena14cs requested a review from rt4914 April 15, 2020 08:09
nikitamarysolomanpvt
nikitamarysolomanpvt reviewed Apr 15, 2020
View reviewed changes
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.

PTAL on suggestions

app/src/main/res/layout-land/hints_and_solution_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/solution_summary.xml Outdated Show resolved Hide resolved
rt4914
rt4914 suggested changes Apr 15, 2020
View reviewed changes
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.

Suggested changes.

app/src/main/java/org/oppia/app/player/state/hintsandsolution/HintsAndSolutionAdapter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/hintsandsolution/HintsAndSolutionAdapter.kt Outdated Show resolved Hide resolved
...c/main/java/org/oppia/app/player/state/hintsandsolution/HintsAndSolutionFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/res/layout-land/hints_and_solution_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/solution_summary.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Apr 15, 2020
@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Apr 15, 2020
rt4914
rt4914 approved these changes Apr 16, 2020
View reviewed changes
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, thanks.

@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Apr 16, 2020
nikitamarysolomanpvt
nikitamarysolomanpvt approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, Please address the toolbar issue as the toolbar title looks smaller when in landscape.

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Apr 16, 2020
nikitamarysolomanpvt
nikitamarysolomanpvt reviewed Apr 16, 2020
View reviewed changes
app/src/main/res/layout-land/hints_and_solution_fragment.xml
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent">

<androidx.appcompat.widget.Toolbar
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow the attributes as per toolbar.xml layout-land.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In one of the PR mentioned by @mschanteltc toolbar textsize is same as portrait 20sp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated status bar color and toolbar size.
Screenshot_20200417-114235

nikitamarysolomanpvt
nikitamarysolomanpvt approved these changes Apr 17, 2020
View reviewed changes
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

@veena14cs
Copy link
Contributor Author

LGTM

Thanks.

@veena14cs veena14cs merged commit 65b1dc8 into develop Apr 17, 2020
@veena14cs veena14cs deleted the hint-solution-landscape branch April 17, 2020 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikitamarysolomanpvt nikitamarysolomanpvt nikitamarysolomanpvt approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@nikitamarysolomanpvt nikitamarysolomanpvt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@veena14cs @nikitamarysolomanpvt @rt4914