-
Notifications
You must be signed in to change notification settings - Fork 69
Improve activity feed design #2634
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
base: master
Are you sure you want to change the base?
Changes from all commits
ef15029
0766080
8fa801a
7ff0f18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /*! | ||
| * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| import type ActivityModel from '../models/ActivityModel.ts' | ||
|
|
||
| import { shallowMount } from '@vue/test-utils' | ||
| import { describe, expect, it } from 'vitest' | ||
| import moment from '@nextcloud/moment' | ||
| import ActivityGroup from '../components/ActivityGroup.vue' | ||
|
|
||
| /** | ||
| * Build a minimal activities prop. The heading only reads the datetime of the | ||
| * first entry to derive its date label, so the rest can be left out. | ||
| */ | ||
| function mountGroup(datetime: string) { | ||
| return shallowMount(ActivityGroup, { | ||
| props: { activities: [{ id: 1, datetime } as unknown as ActivityModel] }, | ||
| }) | ||
| } | ||
|
|
||
| describe('ActivityGroup heading date label', () => { | ||
| it('labels today as "Today" and exposes the full date as the title', () => { | ||
| const wrapper = mountGroup(moment().toISOString()) | ||
| const heading = wrapper.get('.activity-group__heading') | ||
|
|
||
| expect(heading.text()).toBe('Today') | ||
| expect(heading.attributes('title')).toBe(moment().format('LL')) | ||
| }) | ||
|
|
||
| it('labels the previous day as "Yesterday"', () => { | ||
| const wrapper = mountGroup(moment().subtract(1, 'day').toISOString()) | ||
| const heading = wrapper.get('.activity-group__heading') | ||
|
|
||
| expect(heading.text()).toBe('Yesterday') | ||
| expect(heading.attributes('title')).toBe(moment().subtract(1, 'day').format('LL')) | ||
| }) | ||
|
|
||
| it('labels older days with the formatted date and no redundant title', () => { | ||
| const date = moment('2020-01-15T12:00:00') | ||
| const wrapper = mountGroup(date.toISOString()) | ||
| const heading = wrapper.get('.activity-group__heading') | ||
|
|
||
| expect(heading.text()).toBe(date.format('LL')) | ||
| expect(heading.attributes('title')).toBeUndefined() | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,16 +4,18 @@ | |
| --> | ||
|
|
||
| <template> | ||
| <h2 class="activity-group__heading" :title="fullDate"> | ||
| {{ dateText }} | ||
| </h2> | ||
| <ul> | ||
| <ActivityComponent | ||
| v-for="activity in activities" | ||
| :key="activity.id" | ||
| :activity="activity" | ||
| :showPreviews="true" /> | ||
| </ul> | ||
| <section class="activity-group"> | ||
| <h2 class="activity-group__heading" :title="fullDate"> | ||
| {{ dateText }} | ||
| </h2> | ||
| <ul> | ||
| <ActivityComponent | ||
| v-for="activity in activities" | ||
| :key="activity.id" | ||
| :activity="activity" | ||
| :showPreviews="true" /> | ||
| </ul> | ||
| </section> | ||
| </template> | ||
|
|
||
| <script setup lang="ts"> | ||
|
|
@@ -55,14 +57,44 @@ const fullDate = computed(() => { | |
|
|
||
| <style scoped lang="scss"> | ||
| .activity-group { | ||
| // Separate consecutive groups. Inside the <section> (not a margin between them) | ||
| // so the date stays pinned across the gap and the next date docks right as the | ||
| // group ends, instead of the push feeling early. | ||
|
Comment on lines
+60
to
+62
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is weirdly written, can you double-check it? |
||
| padding-block-end: 24px; | ||
|
|
||
| &__heading { | ||
| line-height: 1.5; | ||
| margin-block: 30px 12px; | ||
| position: sticky; | ||
| top: 0; | ||
| z-index: 1; | ||
| // Match the line box to the navigation toggle so the date text lines up with | ||
| // it vertically (centred in a clickable-area-tall row), with no extra | ||
| // whitespace above. Sticking within the per-group <section> makes each new | ||
| // date push the previous one up and out of the way. | ||
| margin-block: 0; | ||
| // Bottom padding only gives the fade more room to complete; because the | ||
| // heading sticks within its <section>, it does not affect when the push starts | ||
| padding-block: 8px 20px; | ||
| // Match the settings-section__name heading size | ||
| font-size: 20px; | ||
| line-height: var(--default-clickable-area); | ||
| // Solid behind the text, then a long, gentle fade to transparent (onset kept | ||
| // at ~32px from the top) so entries dissolve out gradually as they scroll under | ||
| background: linear-gradient(to bottom, var(--color-main-background) 44%, transparent); | ||
|
|
||
| &:first-of-type { | ||
| // Already padding from h1 | ||
| margin-block-start: 0; | ||
| } | ||
| // Indent the heading to clear the app navigation toggle, eased in by the content | ||
| // width. It is the toggle clearance (--app-navigation-padding + clickable-area), | ||
| // less the space the content already has on its left: the centring gutter | ||
| // ((100cqi - column width) / 2, where 100cqi is the content-area width and the | ||
| // column width is --activity-feed-max-width) and its 12px padding. A little | ||
| // breathing room (+8px) minus that 12px padding nets to one negative grid | ||
| // baseline (-4px). Clamped to 0, so it never affects alignment with the entries | ||
| // once the gutter clears the toggle. | ||
| padding-inline-start: calc(max( | ||
| 0px, | ||
| var(--app-navigation-padding, 8px) + var(--default-clickable-area) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need default values for the |
||
| - var(--default-grid-baseline, 4px) | ||
| - max(0px, (100cqi - var(--activity-feed-max-width)) / 2) | ||
| )); | ||
|
Comment on lines
+92
to
+97
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we come up with something simpler? I look quite complex for what we are trying to achieve. |
||
| } | ||
| } | ||
| </style> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,8 @@ | |
| --> | ||
| <template> | ||
| <NcAppContent class="activity-app"> | ||
| <h1 class="activity-app__heading"> | ||
| <!-- Kept for document semantics / screen readers, but visually hidden --> | ||
| <h1 class="activity-app__heading hidden-visually"> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not depend on a global class and have all the needed css here in this file :). |
||
| {{ headingTitle }} | ||
| </h1> | ||
| <NcEmptyContent | ||
|
|
@@ -26,24 +27,26 @@ | |
| </template> | ||
| </NcEmptyContent> | ||
| <div ref="container" class="activity-app__container" @scroll="onScroll"> | ||
| <NcButton | ||
| v-if="newActivitiesAvailable" | ||
| class="activity-app__new-activities-indicator" | ||
| type="button" | ||
| @click="scrollToTop"> | ||
| {{ t('activity', 'New activities') }} | ||
| </NcButton> | ||
| <ActivityGroup v-for="activities, date of groupedActivities" :key="date" :activities="activities" /> | ||
| <!-- Only show if not showing the inital empty content for loading --> | ||
| <NcLoadingIcon | ||
| v-if="hasMoreActivites && allActivities.length > 0" | ||
| :name="t('activity', 'Loading more activities')" | ||
| :size="64" | ||
| class="activity-app__loading-indicator" /> | ||
| <div | ||
| v-else-if="!hasMoreActivites && allActivities.length > 0" | ||
| class="activity-app__loading-indicator"> | ||
| {{ t('activity', 'No more activities.') }} | ||
| <div class="activity-app__content"> | ||
| <NcButton | ||
| v-if="newActivitiesAvailable" | ||
| class="activity-app__new-activities-indicator" | ||
| type="button" | ||
| @click="scrollToTop"> | ||
| {{ t('activity', 'New activities') }} | ||
| </NcButton> | ||
| <ActivityGroup v-for="activities, date of groupedActivities" :key="date" :activities="activities" /> | ||
| <!-- Only show if not showing the inital empty content for loading --> | ||
| <NcLoadingIcon | ||
| v-if="hasMoreActivites && allActivities.length > 0" | ||
| :name="t('activity', 'Loading more activities')" | ||
| :size="64" | ||
| class="activity-app__loading-indicator" /> | ||
| <div | ||
| v-else-if="!hasMoreActivites && allActivities.length > 0" | ||
| class="activity-app__loading-indicator activity-app__loading-indicator--end"> | ||
| {{ t('activity', 'No more activities.') }} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </NcAppContent> | ||
|
|
@@ -349,9 +352,15 @@ watch(props, () => { | |
|
|
||
| <style scoped lang="scss"> | ||
| .activity-app { | ||
| // Max width of the readable content column. Shared with the date heading indent | ||
| // calc in ActivityGroup.vue (inherited), so both stay in sync from one source. | ||
| --activity-feed-max-width: 924px; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you move the var declaration close its usage?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see that 924 is from before 👍 |
||
| display: flex; | ||
| flex-direction: column; | ||
| overflow: hidden; | ||
| // Query container so the date headings can respond to the actual content-area | ||
| // width (which the open app navigation shrinks), rather than the raw viewport | ||
| container: activity-feed / inline-size; | ||
|
|
||
| &__empty-content { | ||
| height: 100%; | ||
|
|
@@ -364,16 +373,26 @@ watch(props, () => { | |
| text-align: center; | ||
| } | ||
|
|
||
| &__loading-indicator--end { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the new class necessary? Can't we add that to the existing class? |
||
| // Breathing room below the end-of-feed message, scaled to the viewport | ||
| margin-block-end: 30vh; | ||
| } | ||
|
|
||
| &__container { | ||
| // Full width so the scrollbar sits at the edge of app-content | ||
| height: 100%; | ||
|
Comment on lines
+373
to
+374
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Full width", but sets the |
||
| overflow-y: scroll; | ||
| } | ||
|
|
||
| &__content { | ||
| // Clamp the readable column and centre it within the full-width scroller | ||
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| height: 100%; | ||
| width: min(100%, 924px); | ||
| max-width: 924px; | ||
| width: min(100%, var(--activity-feed-max-width)); | ||
| max-width: var(--activity-feed-max-width); | ||
| margin: 0 auto; | ||
| padding-inline: 12px; | ||
| overflow-y: scroll; | ||
| } | ||
|
|
||
| &__new-activities-indicator { | ||
|
|
@@ -394,14 +413,5 @@ watch(props, () => { | |
| background-color: var(--color-primary-element-hover); | ||
| } | ||
| } | ||
|
|
||
| &__heading { | ||
| font-weight: bold; | ||
| font-size: 20px; | ||
| line-height: 44px; // to align height with the app navigation toggle | ||
| // Align with app navigation toggle | ||
| margin-top: 1px; | ||
| margin-inline: calc(2 * var(--app-navigation-padding, 8px) + 44px) var(--app-navigation-padding, 8px); | ||
| } | ||
| } | ||
| </style> | ||
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.
No need to compile the assets in every commits.