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

feat(new-resource-form): make visible the required prop fields (DSP-1115) #342

Merged
merged 24 commits into from Jan 19, 2021

Conversation

@flavens
Copy link
Collaborator

@flavens flavens commented Dec 10, 2020

resolves DSP-1115

the PR must be reviewed with the main unpublished version of dsp-ui using yalc

@flavens flavens self-assigned this Dec 10, 2020
@flavens flavens marked this pull request as draft Dec 16, 2020
@flavens flavens marked this pull request as ready for review Dec 18, 2020
@flavens flavens requested a review from mdelez Dec 18, 2020
@mdelez
Copy link
Contributor

@mdelez mdelez commented Jan 5, 2021

Note that we currently need to use yalc in order to use a change made in UI to complete this task.

Loading

@mdelez
Copy link
Contributor

@mdelez mdelez commented Jan 5, 2021

The asterisks next to required fields seems to work well as well as submitting forms with non-required fields not filled out. I can still click on the "save" button when a required field is not filled out which was planned for. However it results in a console error and nothing is presented to the user. I think it would be good if we could somehow jump to the first required field that the user didn't fill out in this case.

Loading

@flavens
Copy link
Collaborator Author

@flavens flavens commented Jan 5, 2021

... I think it would be good if we could somehow jump to the first required field that the user didn't fill out in this case.

I will try this tuto: Medium - How to scroll to the first invalid control once a form has been submitted

Loading

@flavens flavens changed the title DSP-1115 make visible the required prop field to fill in feat(new-resource-form): make visible the required prop fields (DSP-1115) Jan 18, 2021
Loading
@@ -65,6 +72,23 @@ export class SelectPropertiesComponent implements OnInit {
);
}

isPropRequired(propId: string): void {
Copy link
Contributor

@mdelez mdelez Jan 19, 2021

Choose a reason for hiding this comment

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

I think it would make more sense for this method to return a boolean in case we need this logic for other booleans in the future.

Loading

Copy link
Collaborator Author

@flavens flavens Jan 19, 2021

Choose a reason for hiding this comment

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

done in 7858d95

Loading


// check the cardinality to know if the prop is required or not
this.isPropRequired(prop.id);
this.propertyValuesKeyValuePair[prop.id + '-cardinality'] = [this.isRequiredProp ? 1 : 0];
Copy link
Contributor

@mdelez mdelez Jan 19, 2021

Choose a reason for hiding this comment

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

Can you add a comment explaining why we are adding this to the KeyValuePair?

Loading

Copy link
Collaborator Author

@flavens flavens Jan 19, 2021

Choose a reason for hiding this comment

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

done in f131477

Loading

// console.log('createValueComponent', this.createValueComponent);
// this.saveNewValue();
// convert from boolean (1/0) to boolean (true/false)
this.isRequiredProp = !!+this.isRequiredProp;
Copy link
Contributor

@mdelez mdelez Jan 19, 2021

Choose a reason for hiding this comment

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

Maybe you can explain why this is necessary (Input provided by KeyValuePair is stored as a number)

Loading

Copy link
Collaborator Author

@flavens flavens Jan 19, 2021

Choose a reason for hiding this comment

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

done in fb3a064

Loading

@flavens flavens requested a review from mdelez Jan 19, 2021
mdelez
mdelez approved these changes Jan 19, 2021
Copy link
Contributor

@mdelez mdelez left a comment

Looks good!

Loading

@flavens flavens merged commit 5885b04 into main Jan 19, 2021
4 checks passed
Loading
@flavens flavens deleted the wip/dsp-1115-mark-required-fields branch Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants