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

[web] [SIMS #254]Used symbols, updated components to use symbols #274

Merged
merged 5 commits into from
Mar 9, 2021
Merged

[web] [SIMS #254]Used symbols, updated components to use symbols #274

merged 5 commits into from
Mar 9, 2021

Conversation

2011sandeepraj
Copy link
Contributor

No description provided.

@2011sandeepraj 2011sandeepraj changed the title Used symbols, updated compoenents to use symbols [web] [SIMS #254]Used symbols, updated components to use symbols Mar 9, 2021
andrewsignori-aot
andrewsignori-aot reviewed Mar 9, 2021
View reviewed changes
sources/packages/web/src/components/generic/FooterNavigator.vue Outdated
},
setup(props: any) {
const router = useRouter();
console.log("props");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove console logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

andrewsignori-aot
andrewsignori-aot reviewed Mar 9, 2021
View reviewed changes
sources/packages/web/src/constants/routes/RouteConstants.ts Outdated
LOGIN: Symbol(),
STUDENTPROFILE: Symbol(),
STUDENTPROFILEEDIT: Symbol(),
FINANCIALAIDAPPLICATION: Symbol(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please consider using "_" to provide an easier reading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

andrewsignori-aot
andrewsignori-aot reviewed Mar 9, 2021
View reviewed changes
sources/packages/web/src/constants/routes/RouteConstants.ts Outdated
@@ -0,0 +1,17 @@
export const routeConstants = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend segregation between Students and Institutions routes. IMHO it would be easier to read and maintain.
It would be something like "routes.student.FINANCIAL_AID_APPLICATION" and "routes.institution.DASHBOARD".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

andrewsignori-aot
andrewsignori-aot reviewed Mar 9, 2021
View reviewed changes
sources/packages/web/src/views/student/financial-aid-application/ConfirmSubmission.vue Outdated
@@ -2,20 +2,27 @@
<div class="p-component">
<div class="p-card p-m-4">
<Section title="Confirm Submission Page..." />
<FooterNavigator previous="financial-info" />
<FooterNavigator v-bind:previous="financialInfo" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please condider use only the shortcut for v-bind (:previous="financialInfo") instead of (v-bind:previous="financialInfo").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just like using v-bind because it's more explicit. A little old school in this. Hope this is ok

andrewsignori-aot
andrewsignori-aot reviewed Mar 9, 2021
View reviewed changes
sources/packages/web/src/views/student/financial-aid-application/FinancialInfo.vue Outdated
@@ -93,13 +93,17 @@
</ContentGroup>
</Section>
</ContentGroup>
<FooterNavigator previous="select-program" next="confirm-submission" />
<FooterNavigator
v-bind:previous="selectProgram"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please consider using the shortcut for v-bind (:previous) instead (v-bind:previous).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just like using v-bind because it's more explicit. A little old school in this. Hope this is ok

andrewsignori-aot
andrewsignori-aot requested changes Mar 9, 2021
View reviewed changes
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

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

Great work having the names of the routes organized 👍
If we need access to these constantly from the template, we can look into a way to declare the route constants globally.

andrewsignori-aot
andrewsignori-aot reviewed Mar 9, 2021
View reviewed changes
sources/packages/web/jest.config.js
@@ -4,7 +4,8 @@ module.exports = {
"^.+\\.vue$": "vue-jest",
},
globals: {
"ts-jest": {
"ts-jest": {
babelConfig: 'babel.config.js',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please adjust the double quotes to be aligned with the rest of the file.

andrewsignori-aot
andrewsignori-aot approved these changes Mar 9, 2021
View reviewed changes
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

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

Great work with the constants 👍

fwpushan
fwpushan approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@fwpushan fwpushan left a comment

Choose a reason for hiding this comment

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

I recommend name of the route constant should be start with Capital letter.

sources/packages/web/src/constants/routes/RouteConstants.ts Outdated
@@ -0,0 +1,17 @@
export const routeConstants = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Const object name should start with Capital letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@2011sandeepraj 2011sandeepraj merged commit 627e73a into main Mar 9, 2021
@2011sandeepraj 2011sandeepraj deleted the 254b-RefactoringMisc branch March 9, 2021 22:56
sh16011993 added a commit that referenced this pull request Jan 8, 2025
@sh16011993 @bidyashish @andrewsignori-aot
#274 - Merge release v2.2.0 to v2.3.0 (#4216)
a597dee 
**As a part of this PR, release v2.2.0 is merged to v2.3.0**

---------

Co-authored-by: Bidyashish <[email protected]>
Co-authored-by: Andrew Boni Signori <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Jan 8, 2025
@sh16011993 @bidyashish @andrewsignori-aot
#274 - Merge release v2.3.0 to Main Branch (#4217)
88e2bd9 
**As a part of this PR, release v2.3.0 is merged to main. This PR
contains the changes that were directly made into the release branch 2.2
and were updated to the release branch 2.3 when the release branch 2.2
was merged to 2.3**

Co-authored-by: Bidyashish <[email protected]>
Co-authored-by: Andrew Boni Signori <[email protected]>
@bidyashish bidyashish mentioned this pull request Jan 16, 2025
Closed
github-merge-queue bot pushed a commit that referenced this pull request Jan 17, 2025
@bidyashish @sh16011993
@andrewsignori-aot @dheepak-aot
#274 - Merge Release branch v2.3.0 to main (#4260)
da9f0dc 
Release 2.3 from intermediate branch.

#4223 [- Student Bridge Match - PPD
Status Not Importing
](ffb5aa5)

---------

Co-authored-by: Shashank Shekhar <[email protected]>
Co-authored-by: Andrew Boni Signori <[email protected]>
Co-authored-by: Dheepak Ramanathan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsignori-aot andrewsignori-aot andrewsignori-aot approved these changes

@fwpushan fwpushan fwpushan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@2011sandeepraj @fwpushan @andrewsignori-aot