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

Filters - mobile view #27

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Filters - mobile view #27

wants to merge 27 commits into from

Conversation

Adamik10
Copy link
Contributor

@Adamik10 Adamik10 commented Nov 1, 2024

Link to issue

https://reload.atlassian.net/browse/DDFBRA-127

Description

Introduces mobile filters + some small styling changes.

Screenshot of the result

Additional comments or questions

@Adamik10 Adamik10 changed the title Keep spacing between filters the same if they don't fill parent Filters - mobile view Nov 1, 2024
+ Use it on the current interactive elements
Copy link
Contributor

@ThomasGross ThomasGross left a comment

Choose a reason for hiding this comment

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

Some minor things, overall great!

Comment on lines +16 to +21
className={cn([
`focus-visible h-[29px] w-auto self-start whitespace-nowrap rounded-full bg-background-overlay px-4
py-2 text-typo-caption hover:animate-wiggle`,
isActive && "bg-foreground text-background",
classNames,
])}>
Copy link
Contributor

Choose a reason for hiding this comment

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

You dont need an array here

Suggested change
className={cn([
`focus-visible h-[29px] w-auto self-start whitespace-nowrap rounded-full bg-background-overlay px-4
py-2 text-typo-caption hover:animate-wiggle`,
isActive && "bg-foreground text-background",
classNames,
])}>
className={cn(
`focus-visible h-[29px] w-auto self-start whitespace-nowrap rounded-full bg-background-overlay px-4
py-2 text-typo-caption hover:animate-wiggle`,
isActive && "bg-foreground text-background",
classNames,
)}>

@@ -44,48 +45,38 @@ const SearchFiltersColumn = ({
<>
<div
key={facet.name}
className={cn(["relative", !isLast && "min-w-32 flex-1", isLast && "flex-2"])}>
<h3 className="mb-2 text-typo-caption uppercase">{getFacetTranslation(facetFilter)}</h3>
className={cn(["relative ml-[-4px]", !isLast && "min-w-32 flex-1", isLast && "flex-2"])}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
className={cn(["relative ml-[-4px]", !isLast && "min-w-32 flex-1", isLast && "flex-2"])}>
className={cn("relative ml-[-4px]", !isLast && "min-w-32 flex-1", isLast && "flex-2")}>

@@ -1,7 +1,7 @@
import { FacetFieldEnum } from "@/lib/graphql/generated/fbi/graphql"

const search = {
"search.item.limit": 9,
"search.item.limit": 18,
Copy link
Contributor

Choose a reason for hiding this comment

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

9 means faster load times also serverside. Could be nice to keep. Maybe 12 could be the sweetspot

Suggested change
"search.item.limit": 18,
"search.item.limit": 9,

</p>
</button>
</div>
<Icon className={cn("h-8 w-8", isExpanded && "rotate-180")} name="arrow-down" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Im pretty sure "isExpanded" will end up in the class if you're using this time of if statement

Suggested change
<Icon className={cn("h-8 w-8", isExpanded && "rotate-180")} name="arrow-down" />
<Icon className={cn("h-8 w-8", isExpanded ? "rotate-180" : '')} name="arrow-down" />

className={cn(["relative ml-[-4px]", !isLast && "min-w-32 flex-1", isLast && "flex-2"])}>
<h3 className="mb-2 pl-2 text-typo-caption uppercase">
{getFacetTranslation(facetFilter)}
</h3>
<div
className={cn([
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the extra array

Suggested change
className={cn([
className={cn(

@@ -4,6 +4,7 @@ import React, { useEffect, useRef, useState } from "react"
import { SearchFacetFragment, SearchFiltersInput } from "@/lib/graphql/generated/fbi/graphql"
import { cn } from "@/lib/helpers/helper.cn"

import BadgeButton from "../badge/BadgeButton"
import Icon from "../icon/Icon"
Copy link
Contributor

Choose a reason for hiding this comment

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

routes from root

@@ -12,7 +12,7 @@
},
"aliases": {
"components": "@/components/shadcn",
"utils": "@/lib/shadcn/utils",
"utils": "@/lib/helpers",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be changed back

Suggested change
"utils": "@/lib/helpers",
"utils": "@/lib/shadcn/utils",

} from "@/components/accordion/Accordion"
import { SearchFacetFragment, SearchFiltersInput } from "@/lib/graphql/generated/fbi/graphql"

import BadgeButton from "../badge/BadgeButton"
Copy link
Contributor

Choose a reason for hiding this comment

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

import from root

import { SearchFacetFragment, SearchFiltersInput } from "@/lib/graphql/generated/fbi/graphql"

import BadgeButton from "../badge/BadgeButton"
import Icon from "../icon/Icon"
Copy link
Contributor

Choose a reason for hiding this comment

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

import from root


import BadgeButton from "../badge/BadgeButton"
import Icon from "../icon/Icon"
import { Sheet, SheetContent, SheetTrigger } from "../sheet/Sheet"
Copy link
Contributor

Choose a reason for hiding this comment

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

import from root

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