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 images on splash #15459

Merged
merged 15 commits into from May 25, 2022
Merged

Fix images on splash #15459

merged 15 commits into from May 25, 2022

Conversation

vojtechjelinek
Copy link
Member

@vojtechjelinek vojtechjelinek commented May 11, 2022 •
edited

Overview

  1. This PR fixes or fixes part of #N/A.
  2. This PR does the following: Reduces the size of images on the splash page and provides variants of images for different DPIs

Essential 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 linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

test.mp4

Proof of changes on mobile phone

Screencast.2022-05-11.22.16.31.mp4

Proof of changes in Arabic language

Screencast.2022-05-11.21.56.58.mp4

PR Pointers

  • Make sure to follow the instructions for making a code change.
  • If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
  • Never force push. If you do, your PR will be closed.
  • Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

@oppiabot
Copy link

oppiabot bot commented May 11, 2022

Assigning @kevintab95 for the first pass review of this PR. Thanks!

@oppiabot
Copy link

oppiabot bot commented May 11, 2022

Hi @vojtechjelinek, can you complete the following:

  1. The body of this PR is missing the required description, please update the body with a description of what this PR does.
  2. The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
  3. The proof that changes are correct has not been provided, please make sure to upload a image/video showing that the changes are correct. Or include a sentence saying "No proof of changes needed because" and the reason why proof of changes cannot be provided.
    Thanks!

@oppiabot
Copy link

oppiabot bot commented May 13, 2022

Hi @vojtechjelinek, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks!

@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label May 16, 2022
@oppiabot
Copy link

oppiabot bot commented May 16, 2022

Hi @vojtechjelinek, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

kevintab95
kevintab95 approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@kevintab95 kevintab95 left a comment

Choose a reason for hiding this comment

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

No concerns in the codeowner files. Thanks @vojtechjelinek!

@kevintab95 kevintab95 removed their assignment May 16, 2022
@oppiabot oppiabot bot added the PR: LGTM label May 16, 2022
@oppiabot
Copy link

oppiabot bot commented May 16, 2022

Hi @vojtechjelinek, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@oppiabot oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label May 16, 2022
@vojtechjelinek
Copy link
Member Author

@srijanreddy98 Can you please take a pass? The SVGs were minified using https://vecta.io/nano and I always verified that there is no quality degradation.

@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label May 19, 2022
@oppiabot
Copy link

oppiabot bot commented May 19, 2022

Hi @vojtechjelinek, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@oppiabot oppiabot bot added PR: don't merge - HAS MERGE CONFLICTS and removed PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. labels May 19, 2022
@oppiabot
Copy link

oppiabot bot commented May 19, 2022

Hi @vojtechjelinek. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label May 22, 2022
@oppiabot
Copy link

oppiabot bot commented May 22, 2022

Hi @vojtechjelinek, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@oppiabot oppiabot bot removed PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. PR: don't merge - HAS MERGE CONFLICTS labels May 23, 2022
@oppiabot
Copy link

oppiabot bot commented May 23, 2022

Unassigning @srijanreddy98 since they have already approved the PR.

@oppiabot
Copy link

oppiabot bot commented May 23, 2022

Assigning @kevintab95, @aks681 for code owner reviews. Thanks!

@oppiabot
Copy link

oppiabot bot commented May 24, 2022

Hi @vojtechjelinek, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks!

@kevintab95 kevintab95 removed their assignment May 24, 2022
@seanlip seanlip removed their assignment May 24, 2022
aks681
aks681 reviewed May 24, 2022
View reviewed changes
core/templates/pages/classroom-page/classroom-page.component.css
Comment on lines +90 to 97
height: auto;
margin-left: 25vw;
margin-top: -120px;
text-align: center;
}
.oppia-classroom-banner-img {
height: auto;
max-width: 400px;
Copy link
Member

Choose a reason for hiding this comment

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

Can you regenerate the rtl.css file as well?

@aks681 aks681 removed their assignment May 24, 2022
@vojtechjelinek vojtechjelinek merged commit 139c897 into oppia:develop May 25, 2022
@vojtechjelinek vojtechjelinek deleted the improve-speed2 branch May 25, 2022 16:02
@kevintab95 kevintab95 mentioned this pull request Jul 5, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aks681 aks681 aks681 left review comments

@kevintab95 kevintab95 kevintab95 approved these changes

@srijanreddy98 srijanreddy98 srijanreddy98 approved these changes

Assignees

@vojtechjelinek vojtechjelinek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vojtechjelinek @kevintab95 @aks681 @srijanreddy98 @seanlip