-
Notifications
You must be signed in to change notification settings - Fork 14
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
[Web][SIMS-#1110] UI updates for Supporting-Users Account #1456
Conversation
"disabled": false, | ||
"tableView": false, | ||
"modalEdit": false, | ||
"key": "startParentInformationRequest1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can avoid keys ending with numbers like 1. we can add a proper key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that came from the older version. I'll change that. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed keys in the form.
"title": "Start Partner Information Request", | ||
"collapsible": false, | ||
"hideLabel": true, | ||
"key": "startParentInformationRequest2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. we can add a proper key. and pls review all other keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed keys in the form.
<v-card elevation="2" class="mx-auto mt-15" max-width="730px"> | ||
<v-card-text> | ||
<v-row | ||
><v-col cols="15" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm..cols options are from 1 to 12. also you can use md
instead of cols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, sorry. That came from another login page we have here. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh is it? from which login.vue @andrepestana-aot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aest Login page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guru-aot is aest login page part of your PR or your previous PR. we need some refactoring in login page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its was part of my previous story. I am not changing anything now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, are you planning to create a ticket for that? @guru-aot
<v-row | ||
><v-col cols="15" | ||
><v-card-header> | ||
<v-col cols="12"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need v-col
here?
using your BC Services Card. | ||
</p> | ||
<div class="pt-3 pb-2 ml-2"> | ||
<v-row> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need v-row
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need it. Removed.
height="260" | ||
class="mt-8" | ||
alt="An illustration of a woman working at a desk with her laptop. Illustration by Storyset." | ||
src="@/assets/images/happy-parent.svg" /></v-col></v-row></v-card-text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have prettier and eslist enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I do both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that </v-col></v-row></v-card-text
will arrange by the prettier. can you adjust the indentation of these tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @andrepestana-aot added some comments. the Page rendering supportingusersdashboard
form is part of this PR. if so, we need to update formio
with formio-container
Thanks, Ann! Did the changes you mentioned. Please take a look. |
<v-card-text> | ||
<v-row | ||
><v-col cols="9" | ||
><v-card-header> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in other login pages, we are using <v-card-text>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. Done.
@andrepestana-aot I believe that we are missing some labels on the dashboard when checking Figma, as shown below. |
"components": [ | ||
{ | ||
"label": "HTML", | ||
"labelWidth": "", | ||
"labelMargin": "", | ||
"tag": "p", | ||
"className": "", | ||
"attrs": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the customClass category-header-x-large for the welcome content to sync with figma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. Added. Thanks!
@@ -1,9 +1,9 @@ | |||
<template> | |||
<full-page-container> | |||
<formio | |||
<formio-container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I do not see a benefit in doing change. What was the motivation for the change?
@ann-aot I believe that you requested this change in another comment. What is the motivation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the difference between '<formio>' and '<formio-container>'. I thought we were replacing them as @ann-aot asked me to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per our latest dev conversation, there was no effort or recommendation to use formio-container
instead of formio
. The decision was to leave it to the developer, in your case there is absolute no benefit using the formio-container
but as per the agreement, we can leave it as it is because there is no major drawback either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewsignori-aot ok. So, is it resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start, pelase take a look at the comments.
|
That's is different from what I got here: Why is it different for you? I checked on chrome, FF and edge... I think I should increase the bottom margin and align the pic to the bottom anyways. Also what should be the minimum width for the image, @hellolynn-tbtb ? |
</v-btn> | ||
</v-col> | ||
</v-row> | ||
<v-card elevation="2" class="mx-auto mt-15" max-width="730px"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can, as all login mostly follows the same template, like a login-container or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the pattern same for all login pages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but it doesn't look like a container, because if you see a student, there is another card below. but for the upper card. we can create a structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @dheepak-aot, I agree that they are similar enough to a component be created. We can have a tech debit since all login pages are currently changed. We can take care of this on an upcoming opportunity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good first start. Just a minor comment. |
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work @andrepestana-aot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start, looks good 👍
|
||
.img-welcome-background { | ||
@extend .img-background; | ||
background-image: url("../images/image_institution_welcome.svg"); | ||
min-height: $image-min-height; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is min-height not a common property? is it specific for each of image class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for all classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the changes. LGTM 👍
This PR is the first one for the ticket #1110.