-
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
#1757 - Create common container for content in tab #1816
#1757 - Create common container for content in tab #1816
Conversation
Created common container for the contents in tab. However, this does not include the styling changes.
sources/packages/web/src/components/common/OverawardDetails.vue
Outdated
Show resolved
Hide resolved
sources/packages/web/src/components/common/StudentOverawardDetails.vue
Outdated
Show resolved
Hide resolved
sources/packages/web/src/components/layouts/BodyHeaderContainer.vue
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,28 @@ | |||
<template> |
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.
May the template not be simplified as below or something similar?
<template>
<v-card v-if="card">
<slot name="header"></slot>
<slot></slot>
</v-card>
<div v-else>
<slot name="header"></slot>
<slot></slot>
</div>
</template>
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.
<template>
<div class="mb-5">
<!-- Header slot (with and without v-card) -->
<v-card v-if="enableCardView">
<v-container><slot name="header"></slot><slot></slot></v-container>
</v-card>
<div v-else><slot name="header"></slot><slot></slot></div>
</div>
</template>
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 have made the suggested change. There are two slight modifications:
- Wrapped the card / no-card inside a div to give it a
class="mb-5"
to addbottom-margin
after every card / no-card element. - Added a
v-container
inside the v-card
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 terms of code syntax, I would still prefer as below for better readability.
<template>
<div class="mb-5">
<v-card v-if="enableCardView">
<v-container>
<slot name="header"></slot>
<slot></slot>
</v-container>
</v-card>
<div v-else>
<slot name="header"></slot>
<slot></slot>
</div>
</div>
</template>
>No notes found. Please click on create new note to add one.</v-col | ||
></v-row | ||
> | ||
<v-container :fluid="true"> |
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.
our plan was to add the new template to all tab content right?
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, fixed it.
|
||
export default defineComponent({ | ||
props: { | ||
card: { |
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.
can we give a proper name something like isCard
or enableCardView
or maybe something appropriate
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.
Done, changed to enableCardView
<div v-else><slot name="header"></slot></div> | ||
|
||
<!-- Default slot (with and without v-card) --> | ||
<v-card v-if="card" class="mt-n1"><slot></slot></v-card> |
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.
if card==true
then will there be one v-card
for the header and another v-card
for the body?
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.
Fixed it.
@@ -0,0 +1,29 @@ | |||
<template> | |||
<div class="mt-8"> |
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.
just wanted to know, can we use v-container
with fluid
? and maybe we can remove the 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.
Done
required: false, | ||
default: true, | ||
}, | ||
fullWidth: { |
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.
looks like you are not using the prop fullWidth
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.
removed
|
||
export default defineComponent({ | ||
props: { | ||
card: { |
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.
can we give a proper name
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.
done
</div> | ||
</v-card> | ||
<tab-container> | ||
<StudentApplications |
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 can rename it to student-applications
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.
done
sources/packages/web/src/components/layouts/BodyHeaderContainer.vue
Outdated
Show resolved
Hide resolved
'editApplicationAction', | ||
slotProps.data.status, | ||
slotProps.data.id, | ||
<Column |
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 part of this PR but are we going to replace this Column
tag as it is Primevue? @andrewsignori-aot @dheepak-aot @ann-aot @guru-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.
it will be removed, when we replace primevue datable with vuetify data table
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.
The ticket is not precise about the ticket expectations but I was under the impression that we would be changing Student and Institution tabs as part of this effort, but I may be wrong.
@dheepak-aot @guru-aot @andrepestana-aot @ann-aot does someone recall the final goal of the ticket?
You are right @andrewsignori-aot. This ticket was drafted to have the container created for tab but doesn't mention about how far we should go to change the existing tab with new container. In my mind it was only the ministry implementation. May be we (team) can talk quickly to be on same page. |
In my understanding the main goal of the ticket is to create the component (tab-container) to make the spacing uniform among the tabs and deal with different layouts like overawards tab that has multiple "containers". We can keep it only for the ministry student search and create another ticket to apply to other cases. |
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.
Thanks for doing the changes @sh16011993 . LGTM.
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, looks good 👍
Created a common container for contents in the tab. This ensures that there is no jumping effect when the ministry user clicks from one tab to the other ensuring a smooth UI transition. Below are the screenshots:
Profile Tab:
Applications tab:
Restrictions tab:
File Uploads tab:
SIN tab:
Overawards tab:
Notes tab:
Also, checked to make sure that effects of the changes in the shared components are propagated correctly.
Student Application Summary:
Here, the spacing between the Applications title and the datatable is reduced (as a side effect of the above change)