Fix #5755: Added padding to cards to fix alignment issue on mobile view #5968

geetchoudhary · 2018-12-10T10:13:22Z

Explanation

Fixes #5755
Earlier the cards were not aligned with the headers in mobile view.

Screenshots

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.

DubeySandeep · 2018-12-10T10:19:41Z

Hi @geetchoudhary, Thanks for the PR! :)
can you please update the PR title and explanation to follow the pattern mentioned in the checklist?

geetchoudhary · 2018-12-10T10:35:41Z

Hi @geetchoudhary, Thanks for the PR! :)
can you please update the PR title and explanation to follow the pattern mentioned in the checklist?

Done.

codecov-io · 2018-12-10T11:53:20Z

Codecov Report

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

@@            Coverage Diff             @@
##           develop   #5968      +/-   ##
==========================================
- Coverage    45.47%   45.3%   -0.17%     
==========================================
  Files          517     520       +3     
  Lines        30380   30667     +287     
  Branches      4571    4597      +26     
==========================================
+ Hits         13814   13893      +79     
- Misses       16566   16774     +208

Impacted Files	Coverage Δ
...head/pages/skill_editor/SkillEditorStateService.js	`70.4% <0%> (-21.03%)`	⬇️
...plates/dev/head/domain/state/StateObjectFactory.js	`73.33% <0%> (-17.98%)`	⬇️
...ead/domain/skill/EditableSkillBackendApiService.js	`52.72% <0%> (-14.78%)`	⬇️
...ead/domain/exploration/InteractionObjectFactory.js	`75.92% <0%> (-11.04%)`	⬇️
...sions/interactions/Continue/directives/Continue.js	`19.23% <0%> (-0.77%)`	⬇️
...d/pages/question_editor/QuestionEditorDirective.js	`2.77% <0%> (-0.68%)`	⬇️
...es/topic_editor/questions/QuestionsTabDirective.js	`0.52% <0%> (-0.13%)`	⬇️
...d/pages/skill_editor/SkillEditorNavbarDirective.js	`1.72% <0%> (-0.1%)`	⬇️
...ev/head/pages/admin/roles_tab/RolesTabDirective.js	`1.61% <0%> (-0.06%)`	⬇️
...xploration_player/ExplorationPlayerStateService.js	`1.53% <0%> (-0.03%)`	⬇️
... and 23 more

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 b67a19d...c9f7a22. Read the comment docs.

nithusha21

Hi @geetchoudhary I am not sure of the intention of the issue. I have one question. Also, @kevinlee12 could you take a look at this PR (as you filed the corresponding issue).

nithusha21 · 2018-12-10T17:22:19Z

core/templates/dev/head/pages/library/library.html

@@ -235,11 +235,12 @@ <h2 ng-class="{'active': activeGroupIndex === $index}" class="oppia-library-grou
      height: 350px;
      margin-bottom: 72px;
      margin-top: 36px;
-      max-width: 928px;
+      max-width: 975px;


Could you explain the rationale for this change?

Yes, as the maximum width of the div container was fixed to a certain amount adding padding was shrinking the content. Accordingly, I had to increase the max-width to take padding into account.

Does this issue focus only on the left alignment of the cards? If so, I don't think the padding should change for the other 3 directions, and if no extra padding is applied on the top and bottom, then this max-width shouldn't be changed.

Yes, it focuses only on the left alignment and I have only changed the left padding. If the max-width is not increased the scroller(arrow) is pushed to the next line.

Screenshot

Ah I see. Thanks for explaining. Sounds good. I defer further review to @kevinlee12 though

kevinlee12 · 2018-12-12T18:19:23Z

Hi @geetchoudhary, I'm currently pretty swamped with other things at the moment, I'll get to this by the weekend at the latest!

geetchoudhary · 2018-12-12T18:33:32Z

Hi @geetchoudhary, I'm currently pretty swamped with other things at the moment, I'll get to this by the weekend at the latest!

Sure :)

kevinlee12 · 2018-12-16T05:43:36Z

Hi @geetchoudhary, I think there was a bit of misunderstanding of what the issue needed to solve. It was so that the left side of the card would be flushed to the left side of the header, see Rachel's comment here: #5678 (comment) (this was the reason the issue was created). Please let me know if you any questions, I'm happy to answer them 😄

geetchoudhary · 2018-12-16T06:50:47Z

Hi @geetchoudhary, I think there was a bit of misunderstanding of what the issue needed to solve. It was so that the left side of the card would be flushed to the left side of the header, see Rachel's comment here: #5678 (comment) (this was the reason the issue was created). Please let me know if you any questions, I'm happy to answer them

@kevinlee12 , So I just need to align the cards to the left side of the hamburger?

kevinlee12 · 2018-12-16T06:54:08Z

Yup, along with the section headers.

geetchoudhary · 2018-12-16T08:56:50Z

Screenshot

@kevinlee12 , What should be done with the back arrow if we want to allign with the hamburger?

kevinlee12 · 2018-12-17T02:24:02Z

Hmm, good point, do you have any thoughts @rachelwchen?

kevinlee12 · 2018-12-24T07:43:09Z

Hi @geetchoudhary, since Rachel hasn't responded, let's go with the screenshot that you have (we align the left side of the card rather than the arrows). thanks!

geetchoudhary · 2018-12-24T08:02:58Z

Hi @geetchoudhary, since Rachel hasn't responded, let's go with the screenshot that you have (we align the left side of the card rather than the arrows). thanks!

@kevinlee12 , What should I do with the arrow?

geetchoudhary · 2018-12-24T08:03:40Z

Hi @geetchoudhary, since Rachel hasn't responded, let's go with the screenshot that you have (we align the left side of the card rather than the arrows). thanks!

@kevinlee12 , What should I do with the arrow?

If I remove it, How will the user be able to go back?
What I suggest is we align the card and it's header together and then align them with the right side of the hamburger.

Just like in this screenshot

kevinlee12 · 2018-12-24T08:14:09Z

My apologizes, I should have just mentioned that I like what you suggested in your screenshot 😄

geetchoudhary · 2018-12-24T08:34:42Z

My apologizes, I should have just mentioned that I like what you suggested in your screenshot 😄

@kevinlee12 , alright I will push the changes. Thanks :)

kevinlee12 · 2018-12-24T08:36:29Z

thanks!

geetchoudhary · 2018-12-24T09:00:20Z

thanks!

Done. PTAL.

kevinlee12 · 2018-12-24T18:37:14Z

Hi @geetchoudhary, did you push? (last commit that I see is from last week)

geetchoudhary · 2018-12-24T18:55:55Z

Hi @geetchoudhary, did you push? (last commit that I see is from last week)

I made the commit last week for the screenshots. I have pushed the changes today.

kevinlee12 · 2018-12-25T00:12:12Z

Hi @geetchoudhary, I ran this on my iPhone and it's still slightly off (though on Firefox mobile preview it lines up).

geetchoudhary · 2018-12-25T06:55:21Z

Hi @geetchoudhary, I ran this on my iPhone and it's still slightly off (though on Firefox mobile preview it lines up).

I am working on it. Thanks for the screenshot :)

kevinlee12 · 2018-12-26T06:32:07Z

thanks @geetchoudhary! once you are done, feel free to assign this back to me so I know to review 😄

geetchoudhary · 2018-12-28T11:51:36Z

@kevinlee12 , PTAL. Thanks.

kevinlee12

lgtm, thanks @geetchoudhary!

(and sorry for the delay)

geetchoudhary · 2018-12-30T04:27:51Z

@kevinlee12 , Thanks :)

Fixed alignment of cards in mobile view

47e5276

DubeySandeep requested a review from nithusha21 December 10, 2018 10:18

DubeySandeep assigned nithusha21 Dec 10, 2018

geetchoudhary changed the title ~~Fixed (#5755) alignment of cards in mobile view~~ Fix #5755: Added padding to cards to fix alignment issue on mobile Dec 10, 2018

geetchoudhary changed the title ~~Fix #5755: Added padding to cards to fix alignment issue on mobile~~ Fix #5755: Added padding to cards to fix alignment issue on mobile view Dec 10, 2018

nithusha21 reviewed Dec 10, 2018

View reviewed changes

nithusha21 requested a review from kevinlee12 December 10, 2018 17:27

nithusha21 assigned kevinlee12 Dec 10, 2018

aligned header with cards

13dee16

kevinlee12 assigned geetchoudhary and unassigned kevinlee12 and nithusha21 Dec 26, 2018

added support for small devices

c9f7a22

kevinlee12 approved these changes Dec 30, 2018

View reviewed changes

kevinlee12 merged commit ba37cc9 into oppia:develop Dec 30, 2018

geetchoudhary deleted the aligncards branch December 30, 2018 04:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5755: Added padding to cards to fix alignment issue on mobile view #5968

Fix #5755: Added padding to cards to fix alignment issue on mobile view #5968

geetchoudhary commented Dec 10, 2018 •

edited

Loading

DubeySandeep commented Dec 10, 2018

geetchoudhary commented Dec 10, 2018

codecov-io commented Dec 10, 2018 •

edited

Loading

nithusha21 left a comment

nithusha21 Dec 10, 2018

geetchoudhary Dec 10, 2018

nithusha21 Dec 11, 2018

geetchoudhary Dec 11, 2018

nithusha21 Dec 11, 2018

geetchoudhary Dec 11, 2018

kevinlee12 commented Dec 12, 2018

geetchoudhary commented Dec 12, 2018

kevinlee12 commented Dec 16, 2018

geetchoudhary commented Dec 16, 2018 •

edited

Loading

kevinlee12 commented Dec 16, 2018

geetchoudhary commented Dec 16, 2018

kevinlee12 commented Dec 17, 2018

kevinlee12 commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018 •

edited

Loading

kevinlee12 commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018

kevinlee12 commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018

kevinlee12 commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018

kevinlee12 commented Dec 25, 2018

geetchoudhary commented Dec 25, 2018

kevinlee12 commented Dec 26, 2018

geetchoudhary commented Dec 28, 2018

kevinlee12 left a comment

geetchoudhary commented Dec 30, 2018

Fix #5755: Added padding to cards to fix alignment issue on mobile view #5968

Fix #5755: Added padding to cards to fix alignment issue on mobile view #5968

Conversation

geetchoudhary commented Dec 10, 2018 • edited Loading

Explanation

Checklist

DubeySandeep commented Dec 10, 2018

geetchoudhary commented Dec 10, 2018

codecov-io commented Dec 10, 2018 • edited Loading

Codecov Report

nithusha21 left a comment

Choose a reason for hiding this comment

nithusha21 Dec 10, 2018

Choose a reason for hiding this comment

geetchoudhary Dec 10, 2018

Choose a reason for hiding this comment

nithusha21 Dec 11, 2018

Choose a reason for hiding this comment

geetchoudhary Dec 11, 2018

Choose a reason for hiding this comment

nithusha21 Dec 11, 2018

Choose a reason for hiding this comment

geetchoudhary Dec 11, 2018

Choose a reason for hiding this comment

kevinlee12 commented Dec 12, 2018

geetchoudhary commented Dec 12, 2018

kevinlee12 commented Dec 16, 2018

geetchoudhary commented Dec 16, 2018 • edited Loading

kevinlee12 commented Dec 16, 2018

geetchoudhary commented Dec 16, 2018

kevinlee12 commented Dec 17, 2018

kevinlee12 commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018 • edited Loading

kevinlee12 commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018

kevinlee12 commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018

kevinlee12 commented Dec 24, 2018

geetchoudhary commented Dec 24, 2018

kevinlee12 commented Dec 25, 2018

geetchoudhary commented Dec 25, 2018

kevinlee12 commented Dec 26, 2018

geetchoudhary commented Dec 28, 2018

kevinlee12 left a comment

Choose a reason for hiding this comment

geetchoudhary commented Dec 30, 2018

geetchoudhary commented Dec 10, 2018 •

edited

Loading

codecov-io commented Dec 10, 2018 •

edited

Loading

geetchoudhary commented Dec 16, 2018 •

edited

Loading

geetchoudhary commented Dec 24, 2018 •

edited

Loading