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

fix: Fix mobile toolbar when no env-selector, results not wrapping correctly and selected filters badge getting cut #151

Merged
merged 4 commits into from
Jul 21, 2022

Conversation

CachedaCodes
Copy link
Contributor

EX-6625

Pull request template

Motivation and context

Minor fixes for the archetype found while doing the setups for Kenay and Izas.

  • Dependencies. If any, specify:
  • Open issue. If applicable, link:

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?

…rrectly and selected filters badge getting cut

EX-6625
@CachedaCodes CachedaCodes requested a review from a team as a code owner July 19, 2022 08:29
src/components/search/facets/facet-selected-filters.vue Outdated Show resolved Hide resolved
@@ -32,7 +32,7 @@
{{ result.name }}
</h2>
<span v-if="showDescription" class="x-text">{{ result.season }}</span>
<div class="x-list x-list--horizontal x-list--gap-04">
<div class="x-list x-list--horizontal x-list--wrap x-list--justify-center x-list--gap-03">
Copy link
Contributor

Choose a reason for hiding this comment

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

The gap between the result new and old price should be 12px (our 04 option). Why did you decided to change it?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

With this change the result price is always centered, and it shouldn't
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The gap between the result new and old price should be 12px (our 04 option). Why did you decided to change it? image

The change for the gap was done because when the prices wrapped the gap between them was too big. Talked again with Cristina to confirm and she gave the OK to the gap change from -04 to -03.

With this change the result price is always centered, and it shouldn't image

Mixup with Kenay, there the price is center, will fix it.

@@ -34,7 +34,7 @@
</template>

<template #toolbar v-if="$x.query.search">
<MobileToolbar class="x-padding--05 x-padding--top-00" />
<MobileToolbar class="x-padding--left-05 x-padding--bottom-05" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure that this is well done. The icon for the bigger grid cells is confusing in terms of spacing 🤣.

What I can see in figma is that the right spacing should be a little bigger than the left spacing:
image
image

And if we remove the env selector, they are now equal. Doesn't look bad IMHO, but it does not match the designs.
image

Copy link
Contributor Author

@CachedaCodes CachedaCodes Jul 20, 2022

Choose a reason for hiding this comment

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

Talked with Cris and we fixed it the best we could.
The thing is that the button for the column picker in the designs and in the archetype look the same but are different.

In the designs, the column picker icon is a button with a padding of 8px and a gap or padding between elements of 8px. In the archetype, it's just a button with a padding of 16px. That creates a bit of a mismatch on the right side of the last icon. Setting the padding-right of the toolbar to 8px corrects the extra padding of the icon and makes it the same as the designs.

image

Copy link
Contributor

@luismmdev luismmdev left a comment

Choose a reason for hiding this comment

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

The next queries list in the grid is not rendering properly (I think the css is loaded in a different order than in the demo, so it is displayed incorrectly (see attached image).

I think we should add the utility class x-list--horizontal to the next-queries-group.vue in line 20:

      <NextQuery
        #default="{ suggestion: nextQuery }"
        :suggestion="suggestion"
        class="
          x-tag x-tag--pill
          x-list x-list--horizontal x-list--gap-03
          x-padding--bottom-03 x-padding--top-03
          x-border-color--neutral-70
          x-background--neutral-100
        "
      >

image

luismmdev
luismmdev previously approved these changes Jul 21, 2022
Copy link
Contributor

@luismmdev luismmdev left a comment

Choose a reason for hiding this comment

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

good job!

@tiborux tiborux self-requested a review July 21, 2022 09:04
@tiborux
Copy link
Contributor

tiborux commented Jul 21, 2022

We are showing 0 filters selected by default

image

We should not have the badge until we select something

@luismmdev luismmdev merged commit 19e3498 into main Jul 21, 2022
@luismmdev luismmdev deleted the bugfix/EX-6625-minor-styling-fixes branch July 21, 2022 10:52
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.

4 participants