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.

Sign up for GitHub

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

Jump to bottom

feat: redesign - search component #1994

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: redesign - search component #1994

wants to merge 1 commit into from

Conversation

He1DAr
Copy link
Collaborator

@He1DAr He1DAr commented Feb 6, 2025 •
edited
Loading

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Search component redesign

Issue ticket number and link

#1961

Checklist:

  • I have performed a self-review of my code.
  • I have tested the change on desktop and mobile.
  • I have added thorough tests where applicable.
  • I've added analytics and error logging where applicable.

Screenshots (if appropriate):

image
image
image

@vercel Vercel
Copy link

vercel bot commented Feb 6, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 10, 2025 11:14pm
hiro-explorer ❌ Failed (Inspect) Feb 10, 2025 11:14pm

@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from e2ba48d to b353e9a Compare February 6, 2025 20:30
@codecov Codecov
Copy link

codecov bot commented Feb 6, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 1.85185% with 159 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/app/_components/Search/Search.tsx 0.00% 65 Missing and 3 partials ⚠️
src/app/_components/Search/RestultItem.tsx 0.00% 45 Missing and 2 partials ⚠️
src/features/search/search-slice.ts 20.00% 12 Missing ⚠️
..._components/Search/DoubleGradientBorderWrapper.tsx 0.00% 8 Missing ⚠️
src/app/_components/Search/TxTag.tsx 0.00% 8 Missing ⚠️
src/common/fonts.ts 0.00% 7 Missing ⚠️
src/app/_components/Search/Search.stories.tsx 0.00% 4 Missing ⚠️
src/app/_components/Search/SearchLink.tsx 0.00% 3 Missing ⚠️
src/app/layout.tsx 0.00% 1 Missing ⚠️
src/app/txid/[txId]/TxPage.tsx 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from b353e9a to 4009e76 Compare February 6, 2025 20:35
@He1DAr He1DAr requested a review from BLuEScioN February 6, 2025 20:37
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from 4009e76 to f4fcd7a Compare February 6, 2025 20:38
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from f4fcd7a to 2d2fcbe Compare February 6, 2025 20:44
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from 2d2fcbe to 95ea9dc Compare February 6, 2025 21:07
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from 95ea9dc to 7c66c76 Compare February 6, 2025 21:13
@BLuEScioN
Copy link
Collaborator

It would be nice to see a PR description with relevant details

BLuEScioN
BLuEScioN reviewed Feb 6, 2025
View reviewed changes
src/app/_components/Search/RestultItem.tsx Outdated
@@ -0,0 +1,243 @@
import { Flex, FlexProps, Icon } from '@chakra-ui/react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name of this file has a typo

BLuEScioN
BLuEScioN reviewed Feb 6, 2025
View reviewed changes
src/app/_components/Search/RestultItem.tsx Outdated

export function ResultItem({ value }: { value: string }) {
return (
<ResultItemWrapper>
Copy link
Collaborator

@BLuEScioN BLuEScioN Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Every ResultItem in this file basically looks the same to me. It looks like:

<ResultItemWrapper>
      <Flex gap={4} flex={'1 1 auto'} minWidth={0}>
        <SearchLink href={'#'}>
            some text
        </SearchLink>
        <SomeCustomComponentPieces />
        </Flex>
         <ResultItemIcon type={'arrow'} />
    </ResultItemWrapper>

Can you see if it's possible to write one component for all of these and that just takes the tx type and uses it to render the specific <SomeCustomComponentPieces /> it needs?

@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from 7c66c76 to 973811d Compare February 7, 2025 16:07
@ginny-d
Copy link

ginny-d commented Feb 7, 2025

Looking good @He1DAr! I've added some style fixes in the images; hopefully everything is clear, but please ask me if you have any questions.

One additional point: when I focus on the search bar, the "type in" bar appears white and becomes hard to see. Not sure if this is a Storybook preview issue, but just something to check:
Screenshot 2025-02-07 at 15 46 19

Screenshot 2025-02-07 at 16 25 29 Screenshot 2025-02-07 at 16 45 32

@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from 973811d to a468e8c Compare February 7, 2025 16:19
BLuEScioN
BLuEScioN requested changes Feb 7, 2025
View reviewed changes
src/app/_components/Search/Search.tsx

export function SearchResultsWrapper({ children }: { children: ReactNode }) {
return (
<Flex
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whenever using flex and flexdirection column we should be using Stack

src/app/_components/Search/Search.tsx Outdated
flexDirection={'column'}
position={'absolute'}
background={'surfaceSecondary'}
left={'calc(-1 * var(--stacks-spacing-3))'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

does -3 not work? Otherwise, it might work if you put -3 in the spacing config for the theme

src/app/_components/Search/Search.tsx Outdated
left={'calc(-1 * var(--stacks-spacing-3))'}
right={'calc(-1 * var(--stacks-spacing-3))'}
top={'calc(-1 * var(--stacks-spacing-3))'}
borderRadius={'1.125rem'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be using the theme vars

src/app/_components/Search/Search.tsx Outdated
right={'calc(-1 * var(--stacks-spacing-3))'}
top={'calc(-1 * var(--stacks-spacing-3))'}
borderRadius={'1.125rem'}
boxShadow={'0px 16px 32px 0px rgba(184, 180, 176, 0.20)'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's a theme var for this

src/app/_components/Search/Search.tsx Outdated
function DocsLink() {
return (
<SearchLink gap={1} lineHeight={'base'}>
<Icon w={4} h={4}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

no color?

src/app/_components/Search/TxTag.tsx Outdated
export function TxTag({ type }: { type: 'confirmed' | 'pending' | 'failed' }) {
return (
<Flex
display={'flex'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

unneccessary

src/ui/theme/recipes/ButtonRecipe.ts
py: 1.5,
px: 2,
border: '2px solid var(--stacks-colors-new-border-secondary)',
borderRadius: 'md',
Copy link
Collaborator

Choose a reason for hiding this comment

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

redesing.md?

src/ui/theme/recipes/KbdRecipe.ts Outdated
pl: 1.5,
pr: 2,
py: 1,
borderRadius: 'md',
Copy link
Collaborator

Choose a reason for hiding this comment

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

redesign.md?

src/ui/theme/semanticTokens.ts Outdated
search: {
input: {
background: {
value: { base: '{colors.slate.150}', _dark: '{colors.slate.850}' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

unless these values are going into an svg, you can probably remove this and add this object inline

src/features/search/search-slice.ts
@@ -19,8 +24,15 @@ export const searchSlice = createSlice({
setSearchTerm: (state, action: PayloadAction<string>) => {
state.searchTerm = action.payload;
},
setTempSearchTerm: (state, action: PayloadAction<string>) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like either TempSearchTerm and SearchPreviewScrollLeft need better names or they need comments somewhere, as it was difficult through this PR to understand what they did

@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from a468e8c to 2a416fe Compare February 10, 2025 22:07
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from 2a416fe to b8f8c4d Compare February 10, 2025 22:13
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from b8f8c4d to f3d032f Compare February 10, 2025 22:27
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch 2 times, most recently from 3cb5286 to 47b6f92 Compare February 10, 2025 22:29
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from 47b6f92 to ae73218 Compare February 10, 2025 22:59
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from ae73218 to fad0fa2 Compare February 10, 2025 23:07
@He1DAr He1DAr force-pushed the feat/redesign/search-component branch from fad0fa2 to a7fcb8c Compare February 10, 2025 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BLuEScioN BLuEScioN BLuEScioN requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Stacks Explorer
Status: 🆕 New
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@He1DAr @BLuEScioN @ginny-d