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(xo-lite): Add collapsible table row for alarm pool #7641

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

P4l0m4
Copy link
Collaborator

@P4l0m4 P4l0m4 commented May 7, 2024

This component is a table row with a collapsible description.

Capture d'écran 2024-05-14 104449

@P4l0m4 P4l0m4 requested a review from ByScripts May 7, 2024 12:13
@P4l0m4 P4l0m4 self-assigned this May 7, 2024
@P4l0m4 P4l0m4 added XO Lite and removed XO Web Core labels May 7, 2024
@P4l0m4 P4l0m4 changed the title feat(xo-core): Add collapsible table row for alarm pool feat(xo-lite): Add collapsible table row for alarm pool May 7, 2024
@P4l0m4 P4l0m4 marked this pull request as draft May 7, 2024 13:53
@P4l0m4 P4l0m4 marked this pull request as ready for review May 7, 2024 13:59
Copy link
Contributor

@ByScripts ByScripts left a comment

Choose a reason for hiding this comment

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

In addition, the toggle icon + toggle behavior + collapsible row should only be added in case there is an additional description.

@@ -0,0 +1,139 @@
<template>
<tr v-if="isMobile" class="mobile" @click="handleCollapse">
Copy link
Contributor

Choose a reason for hiding this comment

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

You can wrap all tr related to mobile in a <tbody v-if="isMobile">, then all tr related to desktop in a <tbody v-else>

Comment on lines 17 to 19
<div v-tooltip="new Date(parseDateTime(alarm.timestamp)).toLocaleString()" class="ellipsis time">
<RelativeTime :date="alarm.timestamp" />
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

A div can not be a direct child of tr

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed, this component can be removed.

Comment on lines 81 to 82
.mobile,
.desktop {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to replace these classes with a alarm-row class on the wrappers.

</td>
</tr>

<tr v-if="!isDescriptionCollapsedRef && isMobile" class="collapsible-description">
Copy link
Contributor

Choose a reason for hiding this comment

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

This collapsible-description class is not used and can therefore be deleted.

</tr>

<tr v-if="!isDescriptionCollapsedRef && isMobile" class="collapsible-description">
<td class="collapsible-description__content" colspan="5">
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use BEM across the codebase and prefer short, meaningful class names

P4l0m4 and others added 6 commits May 14, 2024 10:10
Co-authored-by: Thierry Goettelmann <thierry@byscripts.info>
Co-authored-by: Thierry Goettelmann <thierry@byscripts.info>
Co-authored-by: Thierry Goettelmann <thierry@byscripts.info>
Co-authored-by: Thierry Goettelmann <thierry@byscripts.info>
…eButton.vue

Co-authored-by: Thierry Goettelmann <thierry@byscripts.info>
@P4l0m4 P4l0m4 requested a review from ByScripts May 14, 2024 09:17
@P4l0m4 P4l0m4 marked this pull request as draft May 14, 2024 09:32
@P4l0m4 P4l0m4 marked this pull request as ready for review May 14, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants