-
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
#3818 - Student Application Summary - Small Screen #4348
#3818 - Student Application Summary - Small Screen #4348
Conversation
792e5ba
to
7fa3847
Compare
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.
Adding the comment here to make it easier to reply.
Below is what I was able to do to convert the PrimeVue table to Vuetify. Please also note the property :mobile="false/false"
that define how the table will be displayed.
Mobile false
mobile true
Please note the below image has the original margins that still need to be adjusted. These changes were applied in my local main branch
.
StudentApplications.vue changes
<content-group>
<toggle-content
:toggled="!applicationsAndCount.results?.length"
message="No applications are currently available."
>
<v-data-table
:headers="StudentApplicationsSummaryHeaders"
:items="applicationsAndCount.results"
:items-per-page="DEFAULT_PAGE_LIMIT"
:items-per-page-options="ITEMS_PER_PAGE"
:mobile="true"
>
<template #[`item.applicationNumber`]="{ item }">
{{ item.applicationNumber }}
</template>
<template #[`item.applicationName`]="{ item }">
<v-btn
v-if="enableViewApplicationOnName"
variant="plain"
@click="$emit('goToApplication', item.id)"
color="primary"
>{{ item.applicationName }}
<v-tooltip activator="parent" location="start"
>Click To View this Application</v-tooltip
>
</v-btn>
<span v-if="!enableViewApplicationOnName"
>{{ item.applicationName }}
</span>
</template>
<template #[`item.studyStartPeriod`]="{ item }">
{{ dateOnlyLongString(item.studyStartPeriod) }} -
{{ dateOnlyLongString(item.studyEndPeriod) }}
</template>
<template #[`item.status`]="{ item }">
<status-chip-application :status="item.status" />
</template>
<template #[`item.actions`]="{ item }">
<span v-if="manageApplication">
<span
v-if="
!(
item.status === ApplicationStatus.Cancelled ||
item.status === ApplicationStatus.Completed
)
"
>
<v-btn
:disabled="sinValidStatus !== SINStatusEnum.VALID"
variant="plain"
color="primary"
class="label-bold"
@click="$emit('editApplicationAction', item.status, item.id)"
append-icon="mdi-pencil-outline"
><span class="label-bold">Edit</span>
<v-tooltip activator="parent" location="start"
>Click To Edit this Application</v-tooltip
>
</v-btn>
<v-btn
:disabled="sinValidStatus !== SINStatusEnum.VALID"
variant="plain"
color="primary"
class="label-bold"
@click="emitCancel(item.id)"
><span class="label-bold">Cancel</span>
<v-tooltip activator="parent" location="start"
>Click To Cancel this Application</v-tooltip
>
</v-btn>
</span>
</span>
<span v-if="enableViewApplication">
<v-btn
variant="outlined"
@click="$emit('goToApplication', item.id)"
>View</v-btn
>
</span>
</template>
</v-data-table>
</toggle-content>
</content-group>
DataTableContract.ts changes
/**
* Student application summary headers.
*/
export const StudentApplicationsSummaryHeaders = [
{ title: "Application Number", sortable: true, key: "applicationNumber" },
{ title: "Name", sortable: false, key: "applicationName" },
{ title: "Submitted", sortable: true, key: "submitted" },
{ title: "Study Period", sortable: true, key: "studyStartPeriod" },
{ title: "Status", sortable: true, key: "status" },
{ title: "Actions", sortable: false, key: "actions" },
];
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 The code sample @andrewsignori-aot .
v-data-table has been updated.
|
||
// Button styles | ||
span.v-btn__content { | ||
white-space: normal !important; | ||
} | ||
|
||
// Mobile-specific styles | ||
.v-container.v-container--fluid.v-locale--is-ltr { | ||
&:has(> .mobile-view) { | ||
padding: 0px !important; | ||
} | ||
} | ||
|
||
.mobile-view { | ||
margin-right: 0px !important; | ||
margin-left: 0px !important; | ||
|
||
.v-card.v-theme--light.v-card--density-default.v-card--variant-elevated.mt-4.p-4.w-100 { | ||
padding: 0px !important; | ||
} | ||
|
||
.v-container { | ||
padding: 5px !important; | ||
width: 100% !important; | ||
max-width: 100% !important; | ||
margin: 0 !important; | ||
} | ||
|
||
.v-row { | ||
margin: 0 !important; | ||
} | ||
|
||
.v-col { | ||
padding: 9px !important; | ||
} | ||
} |
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.
What is the purpose of these styles being added here ?? If that is for the AC Remove the margins on the side for mobile view
then it can be achieved by providing layout class and container class to full page container.
The impact of modifying the vuetify classes in general under mobile view could have undesired effects.
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.
CSS is added to modify mobile view as general css are not able to be override vue general classes.
.v-container.v-container--fluid.v-locale--is-ltr { | ||
&:has(> .mobile-view) { | ||
padding: 0px !important; | ||
} | ||
} |
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.
This can have impacts outside student 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.
it is under mobile-view
css and only application page is added as of now with mobile-view
.
It will only work with 'mobile-view'. css
@sort="paginationAndSortEvent($event)" | ||
:loading="loading" | ||
<toggle-content | ||
:toggled="!applicationsAndCount.results?.length" |
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.
Toggle should be enabled based on total count.
:toggled="!applicationsAndCount.count"
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.
@dheepak-aot this is not a server-side table right now, so all the records are present on the client side and there is no count
available. Please let me know if I am missing 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.
Thanks @andrewsignori-aot ,
No changes made.
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 pointed out by @dheepak-aot API is already accepting the pagination arguments (I missed that), hence can change it.
:field="StudentApplicationFields.ApplicationNumber" | ||
:sortable="true" | ||
header="Application #" | ||
<v-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.
This is a lazy pagination. So we must use v-data-table-server
rather instead of v-data-table
.
Please follow the reference code from here.
<v-data-table-server |
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.
@dheepak-aot I did not see this as lazy because a student may have a few applications over his lifetime. I would recommend helping it as it is right now and saving the server-side pagination for later improvement.
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 Clarification @andrewsignori-aot
No changes made.
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 pointed out by @dheepak-aot API is already accepting the pagination arguments (I missed that), hence can change it.
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.
Update: v-data-table-server
. is updated in table
Looks good after refactor. Added more comments. Please have a look. |
:field="StudentApplicationFields.ApplicationNumber" | ||
:sortable="true" | ||
header="Application #" | ||
<v-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.
👍
sources/packages/web/src/components/common/students/StudentApplications.vue
Outdated
Show resolved
Hide 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.
Thanks for making the changes. Looks good 👍
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 @bidyashish
Acceptance Criteria:
Technical
Iphone 12

Desktop
