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.

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

correction filing (part 1) #22

Merged
merged 3 commits into from
Feb 26, 2020
Merged

Conversation

severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Feb 26, 2020

Issue #: /bcgov/entity#2654

Description of changes:
This is Part 1 of the Correction Filing task, which includes:

  • moved shared components to common folder
  • enabled correction filing in Filing History List menu
  • updated Correction filing page
  • updated Detail Comment to support maxLength prop and to use placeholder instead of label
  • updated Staff Payment component (with Priority and No Fee)
  • replaced <b> with <strong> per SonarCloud

Part 2 will implement:

  • updates to Summary Staff Payment
  • updates to other filings (re: StaffPayment)
  • updates to unit tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the business-filings-ui license (Apache 2.0).

- updated Correction filing page
- updated Detail Comment to support maxLength prop and to use placeholder instead of label
- updated Staff Payment component (with Priority and No Fee)
- TODO: Summary Staff Payment
- TODO: other filings (re: StaffPayment)
- TODO: unit tests
// inform parent of initial validity
this.emitValid(this.value)
}

/**
* Called when prop changes (ie, v-model is updated by parent).
* This method is debounced to prevent excessive validation.
*/
@Watch('value')
@Debounce(300)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this was not implemented in this PR, but this makes me think. Should we have implemented the Debounce decorator on all our listeners? Just a little bit of reading on the topic makes me think it is certainly not a bad thing when trying to optimize performance. No Action required here, just starting a conversation :)

Copy link
Collaborator Author

@severinbeauvais severinbeauvais Feb 26, 2020

Choose a reason for hiding this comment

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

I implemented it here because it needed it (it validated a bunch of rules on every keystroke, which made system responsiveness noticeably slow).

I think we should debounce methods when needed, but I haven't found any other candidates. Do you have something in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No nothing specific, i just know that some of the watchers are triggered constantly and was just thinking ( theoretically ) this is a cool way of optimizing. It's just something i'll keep in mind going forward, if a watcher is a good candidate for Debouncing.

Copy link
Collaborator

@cameron-eyds cameron-eyds left a comment

Choose a reason for hiding this comment

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

Just a few comments/questions. I don't believe anything needs to be actioned at this point. There are some failing unit tests but as we discussed, this PR is to get the work in to clear dependencies for each other and we will resolve the unit tests / updates in a following PR. Approved!

@sonarcloud
Copy link

sonarcloud bot commented Feb 26, 2020

SonarCloud Quality Gate failed.

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 8 Code Smells

No Coverage information No Coverage information
4.0% 4.0% Duplication

Copy link
Contributor

@katiemcgoff katiemcgoff left a comment

Choose a reason for hiding this comment

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

It's a lot of files so I haven't looked too deeply. Approving so it can be combined with Cam's changes and validated in dev.

@katiemcgoff katiemcgoff merged commit a6f9d56 into bcgov:master Feb 26, 2020
thorwolpert pushed a commit to thorwolpert/business-filings-ui that referenced this pull request Mar 16, 2020
* updated doc,dockerfile,dc + added jenkinsfile
* documentation for importing oracle

Signed-off-by: Kial Jinnah <kialj876@gmail.com>
severinbeauvais pushed a commit to severinbeauvais/business-filings-ui that referenced this pull request Mar 19, 2021
* Implemented Unit testing and complete Filing Functionality
* Code Clean up
* Updated Auth Url for pinelines env
* updated logic
* Fixed test
* Updated Unit testing
severinbeauvais added a commit to severinbeauvais/business-filings-ui that referenced this pull request Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants