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 #6903: added required share link directive to exploration save service to force wrapping #6908

Merged
merged 4 commits into from
Jun 13, 2019
Merged

Fix #6903: added required share link directive to exploration save service to force wrapping #6908

merged 4 commits into from
Jun 13, 2019

Conversation

alexewu
Copy link
Contributor

@alexewu alexewu commented Jun 12, 2019 •
edited
Loading

  • editted CSS in post-publish-modal.html to use flex

  • now the elements are aligned in column as seen in the prod version

Explanation

Fixes #6903: added CSS to stying in post-publish-modal.html so that the post publish modal has the close button below the link. This follows the placement of items seen in the prod website.

Fixes #6903: require the share link directive in the exploration save service

Before:
Screen Shot 2019-06-11 at 10 34 58 AM

After:
@import-keshav not sure what you mean by different sizes...I tried making the width of the window much smaller to see the behavior.
Screen Shot 2019-06-12 at 8 56 17 PM

Screen Shot 2019-06-12 at 8 55 05 PM

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 has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
  • The PR follows the style guide.
  • The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
  • The PR is assigned to an appropriate reviewer.
    • If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
    • 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. Do not only request the review but also add the reviewer as an assignee.

Need a reviewer.

@alexewu
Attempted to fix #6903
a7c510f 
* editted CSS in post-publish-modal.html to use flex

* now the elements are aligned in column as seen in the prod version
@alexewu alexewu requested a review from DubeySandeep as a code owner June 12, 2019 17:58
@codecov
Copy link

codecov bot commented Jun 12, 2019 •
edited
Loading

Codecov Report

Merging #6908 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #6908   +/-   ##
========================================
  Coverage    95.32%   95.32%           
========================================
  Files          371      371           
  Lines        50558    50558           
========================================
  Hits         48193    48193           
  Misses        2365     2365

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 419a0f2...8f8f58a. Read the comment docs.

@seanlip seanlip requested a review from apb7 June 12, 2019 20:04
vojtechjelinek
vojtechjelinek suggested changes Jun 12, 2019
View reviewed changes
Copy link
Contributor

@vojtechjelinek vojtechjelinek 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.

The cause of the problems should be fixed with adding appropriate require() into exploration-save.service.ts, as was discussed on Gitter.

@alexewu
Fix #6903
0451698 
* added require of sharing-links directive into the exploration save
service
import-keshav
import-keshav suggested changes Jun 13, 2019
View reviewed changes
Copy link
Contributor

@import-keshav import-keshav left a comment

Choose a reason for hiding this comment

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

Can you please provide screenshots of different screen size.
Thanks

DubeySandeep
DubeySandeep approved these changes Jun 13, 2019
View reviewed changes
Copy link
Member

@DubeySandeep DubeySandeep left a comment

Choose a reason for hiding this comment

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

@alexewu, Thanks for the PR, LGTM!

I've left one simple comment once it's resolved I think we can merge this PR!

core/templates/dev/head/pages/exploration-editor-page/services/exploration-save.service.ts Outdated
@@ -16,6 +16,9 @@
* @fileoverview Service for exploration saving & publication functionality.
*/

require(
'components/common-layout-directives/common-elements/' +
'sharing-links.directive.ts');
Copy link
Member

Choose a reason for hiding this comment

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

Add this after loading-dots.directive.ts (In alphabetical order).

@DubeySandeep
Copy link
Member

@vojtechjelinek, @import-keshav PTAL!

vojtechjelinek
vojtechjelinek approved these changes Jun 13, 2019
View reviewed changes
Copy link
Contributor

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

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

LGTM!

@DubeySandeep
Copy link
Member

@alexewu, Can you please change the PR title before we merge this PR?

@alexewu
Fix #6903
8f8f58a 
* placed require in correct order (alphabetical order)
@alexewu alexewu changed the title Fix #6903: added CSS to put close button below link in post publish modal Fix #6903: added required share link directive to exploration save service to force wrapping Jun 13, 2019
@vojtechjelinek vojtechjelinek merged commit db7b49c into oppia:develop Jun 13, 2019
vojtechjelinek added a commit that referenced this pull request Jun 13, 2019
@vojtechjelinek
Revert "Fix #6903: added required share link directive to exploration…
082a076 
… save service to force wrapping (#6908)"

This reverts commit db7b49c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@import-keshav import-keshav import-keshav approved these changes

@DubeySandeep DubeySandeep DubeySandeep approved these changes

@vojtechjelinek vojtechjelinek vojtechjelinek approved these changes

@apb7 apb7 Awaiting requested review from apb7

Assignees

@import-keshav import-keshav

@alexewu alexewu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Misaligned button and link in published modal
6 participants
@alexewu @DubeySandeep @vojtechjelinek @import-keshav @aks681 @apb7