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 #3972: Fixed the faulty vertical offset #6030

Merged
merged 4 commits into from
Dec 29, 2018
Merged

Fix #3972: Fixed the faulty vertical offset #6030

merged 4 commits into from
Dec 29, 2018

Conversation

mighty-phoenix
Copy link
Contributor

@mighty-phoenix mighty-phoenix commented Dec 25, 2018 •
edited
Loading

Explanation

Fixes #3972 : Fixed the faulty vertical offset by changing the style of div with class "oppia-learner-playlist-tiles" in learner_dashboard.html

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 linter/Karma presubmit checks have passed.
    • These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
  • The PR is made from a branch that's not called "develop".
  • The PR follows the style guide.
  • The PR is assigned to an appropriate reviewer.
    • If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
    • If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

@mighty-phoenix
Copy link
Contributor Author

Behavior before fix:

before

Behavior after fix:

after

@codecov-io
Copy link

codecov-io commented Dec 25, 2018 •
edited
Loading

Codecov Report

Merging #6030 into develop will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6030      +/-   ##
===========================================
- Coverage     45.3%   45.27%   -0.03%     
===========================================
  Files          520      521       +1     
  Lines        30667    30688      +21     
  Branches      4597     4597              
===========================================
+ Hits         13893    13894       +1     
- Misses       16774    16794      +20
Impacted Files Coverage Δ
...lates/dev/head/pages/library/SearchBarDirective.js 1.02% <0%> (-0.1%) ⬇️
...e/templates/dev/head/services/NavigationService.js 4.16% <0%> (ø)
...ts/top_navigation_bar/TopNavigationBarDirective.js 0.88% <0%> (+0.07%) ⬆️

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 92c4ecf...ddd57df. Read the comment docs.

@mighty-phoenix mighty-phoenix removed the request for review from seanlip December 25, 2018 21:34
@nithusha21
Copy link
Contributor

Thanks @rakshitkumarcse for fixing this. This has been a long standing issue. Just curious, what is the rationale behind this fix?

@mighty-phoenix
Copy link
Contributor Author

@nithusha21 The display property of css helps us define the type of rendering box of an element. I found this issue interesting and important for our organization at the same time. So decided to work on it and experiment different solutions. This worked the best.
Thanks!

@bansalnitish
Copy link
Contributor

Hi @rakshitkumarcse @nithusha21, I was looking to test this PR but I'm not able to reproduce the issue. It's written in the issue that it occurs only in certain window size. Could you tell me what browser and what zoom level (or window size) are you using to reproduce this?

Thanks

@mighty-phoenix
Copy link
Contributor Author

@bansalnitish I reproduced the issue on full window size in firefox.

@bansalnitish
Copy link
Contributor

Thanks, I'll check.

@mighty-phoenix
Copy link
Contributor Author

@tjiang11 Can you please review this PR?
Thanks!

bansalnitish
bansalnitish reviewed Dec 28, 2018
View reviewed changes
Copy link
Contributor

@bansalnitish bansalnitish left a comment

Choose a reason for hiding this comment

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

Hi @rakshitkumarcse, This seems to fix the issue, good job:).

I have one question, can we now remove display:block from this line https://github.com/oppia/oppia/pull/6030/files#diff-b75a64eee8fa969ba98f2ac55f98fceeR370 ? If yes, could you please do that? Make sure you test it before and after removing.

Thanks

tjiang11
tjiang11 approved these changes Dec 29, 2018
View reviewed changes
Copy link
Contributor

@tjiang11 tjiang11 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 @rakshitkumarcse ! Will merge after Nitish thinks it's good.

@mighty-phoenix
Copy link
Contributor Author

@bansalnitish @tjiang11 Thanks for the review.
I have removed the display attribute as suggested by @bansalnitish from exploration-summary-tile.
I tested it and it looks the same.
Kindly merge.
Thanks.

@bansalnitish
Copy link
Contributor

LGTM!

@bansalnitish bansalnitish merged commit b25965c into oppia:develop Dec 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bansalnitish bansalnitish bansalnitish left review comments

@tjiang11 tjiang11 tjiang11 approved these changes

Assignees

@tjiang11 tjiang11

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reordering 'play later' explorations in learner dashboard looks strange
6 participants
@mighty-phoenix @codecov-io @nithusha21 @bansalnitish @tjiang11 @seanlip