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 #12910 use app.constants.ts instead of constants.ts #17084

Merged

kevintab95 merged 34 commits into oppia:develop from 462702985:mybranch

Feb 4, 2023

Contributor

462702985 commented Jan 20, 2023 •

edited

Loading

Overview

This PR fixes or fixes part of Use app.constants.ts instead of constants.ts #12910
This PR does the following: use app.constants.ts instead of constants.ts

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 mobile phone

Proof of changes in Arabic language

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.


          use app.constants.ts instead of constants.ts

1b7dfdb

462702985 requested a review from a team as a code owner

January 20, 2023 19:09

462702985 requested a review from a team

January 20, 2023 19:09

462702985 requested a review from a team as a code owner

January 20, 2023 19:09

462702985 requested review from a team

January 20, 2023 19:09

oppiabot bot commented Jan 20, 2023

Hi @462702985, can you complete the following:

The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
Thanks!

462702985 requested review from a team as code owners

January 20, 2023 19:09

462702985 requested review from vojtechjelinek and seanlip

January 20, 2023 19:09

oppiabot bot assigned 462702985

462702985 requested review from kevintab95, BenHenning and heyimShivam

January 20, 2023 19:09

oppiabot bot added the REVIEWERS: Please add changelog label label

oppiabot bot commented Jan 20, 2023

Hi @vojtechjelinek, @seanlip -- could one of you please add the appropriate changelog label to this pull request? Thanks!

oppiabot bot assigned seanlip and vojtechjelinek

462702985 mentioned this pull request

Fix #12910 use app.constants.ts instead of constants.ts #17058

Closed

4 tasks

vojtechjelinek requested changes

View reviewed changes

Member

vojtechjelinek left a comment

Thanks! Took a pass.

core/templates/app.constants.ajs.ts Outdated

Comment on lines 23 to 26

-              import constants from 'assets/constants';
-              for (var constantName in constants) {
-                angular.module('oppia').constant(constantName, constants[constantName]);

Member

vojtechjelinek Jan 20, 2023

I think it is fine to keep import constants from 'assets/constants'; here, so that we do not need to create commonConstants in AppConstants. But add an explanatory comment.

Contributor Author

462702985 Jan 25, 2023

done

core/templates/app.constants.ts Outdated

@@ @@ -20,6 +20,7 @@ import commonConstants from 'assets/constants'; @@
               export const AppConstants = {
                 ...commonConstants,
+                commonConstants,

Member

vojtechjelinek Jan 20, 2023

This should not be needed.

Contributor Author

462702985 Jan 25, 2023

done

core/templates/pages/topic-editor-page/services/subtopic-validation.service.ts Outdated

Comment on lines 22 to 24

		import { AppConstants } from 'app.constants';

		const subtopicValidationConstants = AppConstants.commonConstants;

Member

vojtechjelinek Jan 20, 2023

Again, use AppConstants directly.

Contributor Author

462702985 Jan 25, 2023

done

core/templates/pages/topic-editor-page/modal-templates/create-new-story-modal.controller.ts Outdated

Comment on lines 29 to 30

		import { AppConstants } from 'app.constants';
		const newStoryConstants = AppConstants.commonConstants;

Member

vojtechjelinek Jan 20, 2023

Again, use AppConstants directly.

Contributor Author

462702985 Jan 25, 2023

done

core/templates/pages/topic-editor-page/editor-tab/topic-editor-tab.directive.ts Outdated

Comment on lines 29 to 30

		const topicConstants = AppConstants.commonConstants;

Member

vojtechjelinek Jan 20, 2023

Again, use AppConstants directly.

Contributor Author

462702985 Jan 25, 2023

done

core/templates/pages/teach-page/teach-page.component.ts Outdated

Comment on lines 32 to 33

		const splashConstants = AppConstants.commonConstants;

Member

vojtechjelinek Jan 20, 2023

Again, use AppConstants directly.

Contributor Author

462702985 Jan 25, 2023

done

core/templates/pages/story-editor-page/modal-templates/new-chapter-title-modal.controller.ts Outdated

Comment on lines 28 to 30

		import { AppConstants } from 'app.constants';

		const newChapterConstants = AppConstants.commonConstants;

Member

vojtechjelinek Jan 20, 2023

Again, use AppConstants directly.

Contributor Author

462702985 Jan 25, 2023

done

core/templates/pages/story-editor-page/editor-tab/story-node-editor.directive.ts Outdated

Comment on lines 48 to 50

		import { AppConstants } from 'app.constants';

		const storyNodeConstants = AppConstants.commonConstants;

Member

vojtechjelinek Jan 20, 2023

Again, use AppConstants directly.

Member

vojtechjelinek Jan 20, 2023

ditto elsewhere, also you can probably remove the comment starting with TODO(#9186):

Contributor Author

462702985 Jan 25, 2023

ditto elsewhere, also you can probably remove the comment starting with TODO(#9186):
All comments addressed

vojtechjelinek removed their assignment

vojtechjelinek added PR CHANGELOG: Dev Workflow -- @U8NWXD and removed REVIEWERS: Please add changelog label labels

462702985 and others added 3 commits

January 20, 2023 16:51


          delete commonConstants in AppConstants

276c686


          Merge branch 'oppia:develop' into mybranch

0df16e5


          Merge branch 'mybranch' of https://github.com/462702985/oppia into my…

4e51d50

…branch

seanlip removed their assignment

Contributor Author

462702985 commented Jan 21, 2023

Hi, one frontend time out Disconnected, can you rerun the tests again?

vojtechjelinek requested review from BenHenning and chris7716

February 1, 2023 12:47

462702985 and others added 3 commits

February 1, 2023 11:47


          Merge branch 'develop' into mybranch


          minor change

d3e30c8


          Merge branch 'mybranch' of https://github.com/462702985/oppia into my…

1a7b7e2

…branch

chris7716 approved these changes

View reviewed changes

Contributor

chris7716 left a comment

LGTM. Thanks!

oppiabot bot commented Feb 1, 2023

Assigning @kevintab95, @BenHenning, @heyimShivam for code owner reviews. Thanks!

oppiabot bot assigned BenHenning, heyimShivam and kevintab95

BenHenning approved these changes

View reviewed changes

Member

BenHenning left a comment

Thanks @462702985. Approved for platform parameters code owners.

BenHenning removed their assignment


          Merge branch 'develop' into mybranch

07f0b1c

462702985 removed their assignment

seanlip assigned vojtechjelinek

Member

seanlip commented Feb 3, 2023

@vojtechjelinek @kevintab95 @heyimShivam Please review for codeowners. Thanks!

Member

vojtechjelinek commented Feb 3, 2023

@462702985 Please do not close the comments from reviewers yourself.

vojtechjelinek approved these changes

View reviewed changes

Member

vojtechjelinek left a comment

LGTM!

vojtechjelinek removed their assignment


          Merge branch 'develop' into mybranch

94d6ca5

heyimShivam approved these changes

View reviewed changes

Contributor

heyimShivam left a comment

LGTM

oppiabot bot unassigned heyimShivam

oppiabot bot commented Feb 4, 2023

Unassigning @heyimShivam since they have already approved the PR.

kevintab95 approved these changes

View reviewed changes

oppiabot bot unassigned kevintab95

oppiabot bot commented Feb 4, 2023

Unassigning @kevintab95 since they have already approved the PR.

kevintab95 merged commit b8a1035 into oppia:develop

U8NWXD mentioned this pull request

[Lighthouse Flake]: No node found for selector: mat-option[ng-reflect-value="ADMIN"] #17279

Open

adityanarayanm095 pushed a commit to adityanarayanm095/oppia that referenced this pull request


          Fix oppia#12910 use app.constants.ts instead of constants.ts (oppia#1…

f0d3483

…7084)

* use app.constants.ts instead of constants.ts

* delete commonConstants in AppConstants

* remove TODO(oppia#9186)

* address comments

* linter checks

* Update core/templates/pages/contributor-dashboard-page/opportunities-list-item/opportunities-list-item.component.ts

Co-authored-by: Vojtěch Jelínek <vojtech.jelinek@hey.com>

* Update core/templates/pages/contributor-dashboard-page/services/contribution-opportunities-backend-api.service.ts

Co-authored-by: Vojtěch Jelínek <vojtech.jelinek@hey.com>

* delete unnecessary changes

* minor bug

* minor bug

* minor change

---------

Co-authored-by: Vojtěch Jelínek <vojtech.jelinek@hey.com>

462702985 deleted the mybranch branch

February 16, 2023 16:43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

heyimShivam heyimShivam approved these changes

chris7716 chris7716 approved these changes

BenHenning BenHenning approved these changes

kevintab95 kevintab95 approved these changes

vojtechjelinek vojtechjelinek approved these changes

Labels

None yet