oppia icon indicating copy to clipboard operation
oppia copied to clipboard

Published 20 hours ago • verified publisher icon oppia

Class name change in profile-page.component.js file to avoid class name collision

Open karenbanci opened this issue 2 years ago • 9 comments

Overview

  1. This PR fixes or fixes part of #15988.
  2. This PR does the following: Class name change in profile-page.component.js file to avoid class name collision

Essential Checklist

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

Proof that changes are correct

Before: Note that the div on localhost and oppia.org has the same class name, but the style is different. oppia.org is styled responsive and localhost is not. Screen Shot 2022-09-04 at 11 29 58 PM

After: Made class name changes from "oppia-profile-picture-container" to "oppia-editor-profile-picture-container", no longer superimposing one css style over another Screen Shot 2022-09-05 at 12 41 20 AM

Proof of changes on mobile phone

Screen Shot 2022-09-05 at 12 46 33 AM

References:

  • Chrome: https://developer.chrome.com/docs/devtools/device-mode/

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.

karenbanci avatar Sep 05 '22 07:09 karenbanci

Hi @ashutoshc8101, could you please add the appropriate changelog label to this pull request? Thanks!

oppiabot[bot] avatar Sep 05 '22 07:09 oppiabot[bot]

CI tests are failing due to flakiness, I believe. The errors are not related to the changes of this PR. Screen Shot 2022-09-05 at 5 00 27 PM

Can you please rerun the test?

karenbanci avatar Sep 06 '22 00:09 karenbanci

Hi @karenbanci, 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[bot] avatar Sep 08 '22 05:09 oppiabot[bot]

@karenbanci #15653 brought up this issue which seems weird to me because it only moved the styles from the component.html file to a CSS file. Can you investigate how moving these styles brought about this issue? Your approach is acceptable but it does not fix the original problem nor does it track how #15653 brought it up.

ashutoshc8101 avatar Sep 13 '22 16:09 ashutoshc8101

@karenbanci #15653 brought up this issue which seems weird to me because it only moved the styles from the component.html file to a CSS file. Can you investigate how moving these styles brought about this issue? Your approach is acceptable but it does not fix the original problem nor does it track how #15653 brought it up.

image

Removing these styles does fix the UI bug.

ashutoshc8101 avatar Sep 13 '22 16:09 ashutoshc8101

@karenbanci #15653 brought up this issue which seems weird to me because it only moved the styles from the component.html file to a CSS file. Can you investigate how moving these styles brought about this issue? Your approach is acceptable but it does not fix the original problem nor does it track how #15653 brought it up.

@ashutoshc8101 I fixed the problem, the print you quoted is not referring to this PR, I ask you to check my modifications here https://github.com/oppia/oppia/pull/15990/files Screen Shot 2022-09-13 at 9 23 50 AM

karenbanci avatar Sep 13 '22 16:09 karenbanci

Hi @karenbanci, 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[bot] avatar Sep 17 '22 09:09 oppiabot[bot]

Assigned @ashutoshc8101 since it looks like they need to take the next step. @karenbanci please make sure to assign people when you want them to take action, since most people use the list of PRs they're assigned to as their TODO list. See this note at the bottom of the first comment in the PR:

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.

U8NWXD avatar Sep 20 '22 20:09 U8NWXD

Hi @karenbanci, 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[bot] avatar Sep 21 '22 16:09 oppiabot[bot]

@U8NWXD Sorry, but I'm not allowed to assign something

karenbanci avatar Sep 22 '22 23:09 karenbanci

I'm having problem with this test @seanlip PTAL

Screen Shot 2022-09-22 at 10 45 52 PM

Is this a flaky test?

karenbanci avatar Sep 23 '22 05:09 karenbanci

Hi @karenbanci, possibly -- a list of flaky tests can be found here: https://github.com/orgs/oppia/projects/35. This one might be related to the italiano one.

@ShivamJhaa could you please advise?

Thanks for checking!

seanlip avatar Sep 23 '22 05:09 seanlip

Unassigning @karenbanci since a re-review was requested. @karenbanci, please make sure you have addressed all review comments. Thanks!

oppiabot[bot] avatar Sep 23 '22 05:09 oppiabot[bot]

Also, @karenbanci, re assigning folks, the message that @U8NWXD quoted explains how to do it. You can just leave a comment like "{{Question/comment}} @{{reviewer_username}} PTAL" and Oppiabot will sort it out for you.

@aks681, I'm also assigning you per our discussion in case you can give some feedback on the approach here. Thanks!

seanlip avatar Sep 23 '22 05:09 seanlip

Hi @karenbanci, 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[bot] avatar Sep 25 '22 05:09 oppiabot[bot]

Hi @karenbanci, possibly -- a list of flaky tests can be found here: https://github.com/orgs/oppia/projects/35. This one might be related to the italiano one.

@ShivamJhaa could you please advise?

Thanks for checking!

Yes, looks like a flake (same as the italiano). Updated the issue to include this case as well. Thanks.

ShivamJhaa avatar Sep 25 '22 06:09 ShivamJhaa

Unassigning @aks681 since they have already approved the PR.

oppiabot[bot] avatar Sep 25 '22 18:09 oppiabot[bot]

Unassigning @ashutoshc8101 since they have already approved the PR.

oppiabot[bot] avatar Sep 26 '22 09:09 oppiabot[bot]

Hi @aks681, this PR is ready to be merged. Author of this PR does not have permissions to merge this PR. Before you merge it, please make sure that there are no pending comments that require action from the author's end. Thanks!

oppiabot[bot] avatar Sep 26 '22 09:09 oppiabot[bot]

Note: @aks681 mentioned this to me in chat: "I didn't change the css style itself, just moved it to .css file. There is another class with same name in oppia.css at max-width 610px, which is what I think causes the issue as that may have got overwritten when everything was in .css file"

Also @karenbanci it looks like you have all approvals so I'm merging this. Thanks for your PR and for fixing the issue!

seanlip avatar Sep 26 '22 15:09 seanlip

@aks681 @seanlip Thank you

karenbanci avatar Sep 26 '22 15:09 karenbanci