Skip to content

65 wizard validation error message#100

Merged
khoadnguyen merged 15 commits intodevfrom
65-wizard-validation-erro-message
Oct 9, 2018
Merged

65 wizard validation error message#100
khoadnguyen merged 15 commits intodevfrom
65-wizard-validation-erro-message

Conversation

@iamalx
Copy link
Copy Markdown
Collaborator

@iamalx iamalx commented Oct 8, 2018

#65 I did major layout and style modifications to both demo slides and questionnaire. I also spend a good amount of time on the questions, I set up the main logic for the question cards and the radio alert. I am satisfied by the way the questions are organized and displayed. However, I was having a hard time validating the radio alerts since it is not in the template, I only call the radio alert in an a tag, but it does not have an input tag so validating it with form group is impossible, i think. To make it worse, some questions are required only if they meet a condition, so I was trying to make the required attribute in the template interactive, but couldn't. And I also tried to make the "Validators.required" parameter interactive by using "Validators["string variable"], but it did not work.

@iamalx iamalx added the blocker This issue requires more assistance from others label Oct 8, 2018
@iamalx iamalx added this to the Week 4 milestone Oct 8, 2018
@iamalx iamalx self-assigned this Oct 8, 2018
@iamalx iamalx requested a review from khoadnguyen October 8, 2018 19:20
@iamalx iamalx closed this Oct 8, 2018
@iamalx iamalx reopened this Oct 8, 2018
Comment thread src/pages/profile/profile.scss Outdated
.name {
font-size: 30px;
}
.section {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These profile style updates should be in their own commit. I'll allow it this one time, but your issues/pr should be separate going forward.

Comment thread src/theme/variables.scss
light: #f4f4f4,
dark: #222
);
// $alert-min-width: (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you test out styles and they don't work, it's best not to pollute a global file with it. This is relatively small change, so we can take it out but just keep that in mind for the future.

@khoadnguyen
Copy link
Copy Markdown
Contributor

Thanks for getting this to me for review. I think the best course of action would be for everyone on the team to look at it the issue you're facing together and we can tackle it. It would also be a good teachable moment. I would say be prepared to talk about the challenges you faced and things you tried to remedy the issue. Good effort though, seems like you made progress. 👍

@khoadnguyen khoadnguyen added the code review This PR is ready for code review label Oct 8, 2018
@khoadnguyen khoadnguyen merged commit 4aaf791 into dev Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocker This issue requires more assistance from others code review This PR is ready for code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants