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

Form/action refactoring and other updates #2666

Merged
merged 10 commits into from
Mar 14, 2023

Conversation

dpwatrous
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #2666 (9eb0c62) into main (aa2e3fc) will increase coverage by 0.34%.
The diff coverage is 76.46%.

❗ Current head 9eb0c62 differs from pull request most recent head 9b62d70. Consider uploading reports for the commit 9b62d70 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2666      +/-   ##
==========================================
+ Coverage   65.81%   66.15%   +0.34%     
==========================================
  Files        1167     1199      +32     
  Lines       32528    33303     +775     
  Branches     5929     6128     +199     
==========================================
+ Hits        21409    22033     +624     
- Misses      11005    11138     +133     
- Partials      114      132      +18     
Impacted Files Coverage Δ
...ages/common/src/ui-common/http/mock-http-client.ts 73.11% <0.00%> (-3.29%) ⬇️
packages/service/src/ui-service/constants.ts 100.00% <ø> (ø)
...ackages/service/src/ui-service/odata/odata-util.ts 80.00% <0.00%> (-3.34%) ⬇️
...-service/subscription/fake-subscription-service.ts 18.75% <0.00%> (ø)
...ui-service/storage/fake-storage-account-service.ts 16.66% <12.50%> (-0.99%) ⬇️
...ct/src/ui-react/environment/browser-environment.ts 38.70% <16.66%> (-8.17%) ⬇️
...ckages/common/src/ui-common/logging/mock-logger.ts 72.41% <20.00%> (-4.95%) ⬇️
...d/src/ui-playground/demo/form/action-form-demo.tsx 22.53% <23.80%> (-10.80%) ⬇️
.../src/ui-service/certificate/certificate-service.ts 48.48% <33.33%> (+1.60%) ⬆️
...ice/src/ui-service/certificate/certificate-util.ts 75.00% <33.33%> (ø)
... and 63 more

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa2e3fc...9b62d70. Read the comment docs.

@dpwatrous dpwatrous force-pushed the dpwatrous/parameter-dependencies branch 2 times, most recently from bcb0ed0 to 19dd662 Compare March 14, 2023 12:36
- Properly display form validation errors
- Replaced "Parameter types" with Parameter classes
- Form dependencies
- Dynamic properties
- Move update logic into layout components
- Support action data loading
- New parameters and form controls
Copy link
Member

@gingi gingi left a comment

Choose a reason for hiding this comment

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

Some opportunities for refactoring in the future, but this is great!

}

private _validate(): ValidationStatus {
if (this.value != null && Array.isArray(this.value)) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems that if value is not null nor an array, it should raise an error, no?

if (errorCount === 1 && lastErrorMsg) {
errorMsg = lastErrorMsg;
} else {
errorMsg = `${errorCount} ${
Copy link
Member

Choose a reason for hiding this comment

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

Might want to generalize this a bit for localization (because "X errors found" might not be grammatically correct in other languages): errorCount === 1 ? `1 error found` : `${errorCount} errors found` .

warningMsg = lastWarningMsg;
} else {
warningMsg = `${warningCount} ${
warningCount === 1 ? "warning" : "warnings"
Copy link
Member

Choose a reason for hiding this comment

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

See above

* Creates a standalone parameter with a backing form. Useful for using
* form controls outside the context of an entire form
*/
export function createParam<
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this more: Why not just put use the constructor (e.g., put the logic in AbstractParameter's constructor)?

@dpwatrous dpwatrous merged commit 4938948 into main Mar 14, 2023
@dpwatrous dpwatrous deleted the dpwatrous/parameter-dependencies branch March 14, 2023 15:23
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.

None yet

2 participants