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: collapse desktop header in scrolling #288

Conversation

lauramargar
Copy link
Contributor

EMP-1003

Motivation and context

Provide the user a better visualization with more space for the results.
We have created a new component (desktop-sub-header) where we add all the elements that we want to be hidden (related-tags, desktop-toolbar and selected-filters). We have also created the animation that we separated into 2 files, collapse-heigth-animation.scss and is-scrolling-up.mixin.ts.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that causes existing functionality to not work as expected)
  • Change requires a documentation update

What is the destination branch of this PR?

  • Main
  • Other. Specify:

How has this been tested?

When you scroll over the result list, the related tags, toolbar and selected filters should hide.

Replace DesktopHeaderFullPredictive with DesktopHeaderFloatingPredictive in desktop and RelatedTagsFullPredictive with RelatedTagsFloatedPredictive in desktop-sub-header if you want to test this feature with the floating empathize.

@lauramargar lauramargar requested a review from a team as a code owner June 7, 2023 10:39
@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@@ -90,7 +100,9 @@
NoResultsMessage: () => import('../search').then(m => m.NoResultsMessage),
SelectedFilters: () => import('../search').then(m => m.SelectedFilters),
SpellcheckMessage: () => import('../search').then(m => m.SpellcheckMessage)
}
},

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove blank space


<script lang="ts">
import { defineComponent } from 'vue';
//import RelatedTagsFloatedPredictive from './related-tags-floated-predictive.vue';
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment

export default defineComponent({
components: {
DesktopToolbar,
//RelatedTagsFloatedPredictive,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a wrapper Vue component (not like the the animations components we have in X, but one that applies the classes with a prop)

>
<div class="x-layout__sub-header-content">
<DesktopSearchboxAlign class="x-layout-container">
<RelatedTagsFullPredictive />
Copy link
Contributor

Choose a reason for hiding this comment

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

This way if we have to change the type of predictive we use we'll have to change it in two places, change the components structure so it's only one place (maybe with a prop to make things easier, something like this flowchart)

@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@@ -0,0 +1,44 @@
<template>
<div
class="x-collapse"
Copy link
Contributor

Choose a reason for hiding this comment

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

collapse is too generic, change to collapse-height

},
mixins: [IsScrollingUp],
props: {
hasScrolled: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This prop is not being used

Comment on lines 3 to 5
<div>
<DesktopHeaderFullPredictive />
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This wrapping is not needed

Suggested change
<div>
<DesktopHeaderFullPredictive />
</div>
<DesktopHeaderFullPredictive />

Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to collapse-height-animation

Comment on lines 2 to 22
<CollapseHeigthAnimation
class="x-layout-max-width-md desktop:x-layout-min-margin-32 large:x-layout-max-width-lg large:x-layout-min-margin-48"
:isExtended="hasScrolled"
>
<div class="x-layout__sub-header-content">
<DesktopSearchboxAlign>
<div class="x-layout-item" :class="{ 'x-grid x-grid-cols-6': !isFullPredictive }">
<LocationProvider location="predictive_layer">
<RelatedTags v-if="$x.relatedTags.length > 0" class="x-pb-24" />
</LocationProvider>
</div>
</DesktopSearchboxAlign>

<div v-if="!$x.redirections.length && hasSearched">
<DesktopToolbar />
</div>
<div v-if="$x.totalResults > 0 && hasSearched && $x.selectedFilters.length">
<SelectedFilters class="x-py-16" />
</div>
</div>
</CollapseHeigthAnimation>
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the MaxDesktopWidthItem component instead of the CSS classes.
Also x-layout__sub-header-content is not needed anymore.

Suggested change
<CollapseHeigthAnimation
class="x-layout-max-width-md desktop:x-layout-min-margin-32 large:x-layout-max-width-lg large:x-layout-min-margin-48"
:isExtended="hasScrolled"
>
<div class="x-layout__sub-header-content">
<DesktopSearchboxAlign>
<div class="x-layout-item" :class="{ 'x-grid x-grid-cols-6': !isFullPredictive }">
<LocationProvider location="predictive_layer">
<RelatedTags v-if="$x.relatedTags.length > 0" class="x-pb-24" />
</LocationProvider>
</div>
</DesktopSearchboxAlign>
<div v-if="!$x.redirections.length && hasSearched">
<DesktopToolbar />
</div>
<div v-if="$x.totalResults > 0 && hasSearched && $x.selectedFilters.length">
<SelectedFilters class="x-py-16" />
</div>
</div>
</CollapseHeigthAnimation>
<CollapseHeigthAnimation :isExtended="hasScrolled">
<MaxDesktopWidthItem>
<DesktopSearchboxAlign>
<div class="x-layout-item" :class="{ 'x-grid x-grid-cols-6': !isFullPredictive }">
<LocationProvider location="predictive_layer">
<RelatedTags v-if="$x.relatedTags.length > 0" class="x-pb-24" />
</LocationProvider>
</div>
</DesktopSearchboxAlign>
<div v-if="!$x.redirections.length && hasSearched">
<DesktopToolbar />
</div>
<div v-if="$x.totalResults > 0 && hasSearched && $x.selectedFilters.length">
<SelectedFilters class="x-py-16" />
</div>
</MaxDesktopWidthItem>
</CollapseHeigthAnimation>

Comment on lines 10 to 13
@XOn('UserChangedScrollDirection')
scrollDirectionChanged(direction: ScrollDirection): void {
this.stopsScrollDown = direction === 'UP';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will listen to any scroll direction change managed by the scroll module. We should be concerned only by changes on the main scroll. Change the implementation to using the state of the main scroll only.

Suggested change
@XOn('UserChangedScrollDirection')
scrollDirectionChanged(direction: ScrollDirection): void {
this.stopsScrollDown = direction === 'UP';
}
@State('scroll', 'data')
public scrollPositionsMap!: Dictionary<ScrollComponentState>;
protected get mainScrollPosition(): number {
return this.scrollPositionsMap['main-scroll']?.position;
}

Comment on lines 22 to 26
if (this.stopsScrollDown || position < this.scrollOffset) {
this.isScrollingUp = true;
} else if (!this.stopsScrollDown && position > this.scrollOffset) {
this.isScrollingUp = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the main scroll state and apply the changes to the property of isScrollingUp.

Suggested change
if (this.stopsScrollDown || position < this.scrollOffset) {
this.isScrollingUp = true;
} else if (!this.stopsScrollDown && position > this.scrollOffset) {
this.isScrollingUp = false;
}
const isScrollingUp = this.scrollPositionsMap['main-scroll']?.direction === 'UP';
if (isScrollingUp || this.mainScrollPosition < this.scrollOffset) {
this. hasScrolledPastThreshold = false;
} else if (!isScrollingUp && this.mainScrollPosition > this.scrollOffset) {
this. hasScrolledPastThreshold = true;
}```

Comment on lines 15 to 16
@XOn(['UserScrolled'])
scrollLength(position: number): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Convert to watcher and rename function

Suggested change
@XOn(['UserScrolled'])
scrollLength(position: number): void {
@Watch('mainScrollPosition', { deep: true })
updateHasScrolledPastThreshold(): void {

Comment on lines 35 to 36
protected get hasScrolled(): boolean {
return this.isScrollingUp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Update naming

Suggested change
protected get hasScrolled(): boolean {
return this.isScrollingUp;
protected get hasScrolled(): boolean {
return this.hasScrolledPastThreshold;

Copy link
Contributor

Choose a reason for hiding this comment

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

Rename mixing name to something like has-scroll-past-threshold

@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@jenkins-empathy
Copy link

PR #288 preview deployed in https://x.test.empathy.co/preview/288/index.html

@CachedaCodes CachedaCodes merged commit 979ff84 into main Jun 13, 2023
@CachedaCodes CachedaCodes deleted the feature/EMP-1003-as-a-shopper-i-want-the-desktop-header-to-collapse-in-scrolling branch June 13, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants