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

[Component] Search #34

Closed
wants to merge 23 commits into from
Closed

[Component] Search #34

wants to merge 23 commits into from

Conversation

elisegriset92
Copy link
Contributor

@elisegriset92 elisegriset92 commented Jul 13, 2022

❓ Types of changes

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • πŸ“¦ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Closes #23

πŸ“ Checklist

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes

@elisegriset92 elisegriset92 changed the title [Component] Search component [Component] Search Jul 13, 2022
@elisegriset92 elisegriset92 self-assigned this Jul 13, 2022
@elisegriset92 elisegriset92 added Feature Type: New Feature VueJS Tech: VueJS labels Jul 13, 2022
@elisegriset92 elisegriset92 marked this pull request as ready for review July 13, 2022 16:41
Copy link
Contributor

@ga-devfront ga-devfront left a comment

Choose a reason for hiding this comment

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

Thank you for your work ! Here is a small review of what I think could be improved.
Don't forget to rebase for resolve conflicts.

packages/components/search/src/search.ts Outdated Show resolved Hide resolved
packages/components/search/src/search.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@ga-devfront ga-devfront left a comment

Choose a reason for hiding this comment

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

Really thank you for your work ! I have just a little one proposition for improving your component.

packages/components/search/src/search.ts Outdated Show resolved Hide resolved
ga-devfront
ga-devfront previously approved these changes Jul 25, 2022
Copy link
Contributor

@ga-devfront ga-devfront left a comment

Choose a reason for hiding this comment

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

Thank you for all your work, I think we can merge it !
Just don't forget to rebase.

@ga-devfront
Copy link
Contributor

@aAmorim27 possible de complΓ©ter ma review pour Γͺtre sur de ne rien avoir oubliΓ© avant de merge ?

Copy link
Contributor

@aAmorim27 aAmorim27 left a comment

Choose a reason for hiding this comment

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

I'm having an Uncaught RangeError: Maximum call stack size exceeded when i delete the text in search after pressing the submit button
Otherwise there is small improvements to do and it's seems that the confirm button is missing from html/css snippets of the documentation

packages/components/search/src/search.vue Outdated Show resolved Hide resolved
packages/components/search/src/search.vue Show resolved Hide resolved
packages/components/search/src/search.vue Outdated Show resolved Hide resolved
packages/components/search/src/search.ts Outdated Show resolved Hide resolved
packages/components/search/src/search.vue Outdated Show resolved Hide resolved
packages/components/search/src/search.vue Outdated Show resolved Hide resolved
Comment on lines 45 to 49
setup() {
return { args }
},
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
setup() {
return { args }
},
setup() {
const myValue = ref('')
return { args, myValue }
},

Add a v-model on this story or otherwise we won't be able to use the search because the value will dispear when unfocusing the element

@elisegriset92
Copy link
Contributor Author

@aAmorim27 need other review ?

@ga-devfront
Copy link
Contributor

@aAmorim27 need other review ?

yes I think is cool if you can review it

aAmorim27
aAmorim27 previously approved these changes Sep 19, 2022
Copy link
Contributor

@aAmorim27 aAmorim27 left a comment

Choose a reason for hiding this comment

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

Ok for me πŸ‘ , just some quick fixes to do on the storybook docs

},
autocomplete: {
description: 'Set the autocomplete mode of the input',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The rounded prop is missing in the controls list

packages/components/search/stories/search.stories.ts Outdated Show resolved Hide resolved
packages/components/search/stories/search.stories.ts Outdated Show resolved Hide resolved
packages/components/search/stories/search.stories.ts Outdated Show resolved Hide resolved
packages/components/search/stories/search.stories.ts Outdated Show resolved Hide resolved
packages/components/search/stories/search.stories.ts Outdated Show resolved Hide resolved
packages/components/search/stories/search.stories.ts Outdated Show resolved Hide resolved
packages/components/search/stories/search.stories.ts Outdated Show resolved Hide resolved
Comment on lines 214 to 233
<!--VueJS Snippet-->
<puik-search v-model="myValue" rounded :autocomplete="'test'"/>

<!--HTML/CSS Snippet-->
<div class="puik-search">
<div class="puik-search__wrapper puik-search__wrapper--rounded-input">
<input class="puik-search__field" type="text" rounded :autocomplete="'test'"/>
</div>
</div>
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
<!--VueJS Snippet-->
<puik-search v-model="myValue" rounded :autocomplete="'test'"/>
<!--HTML/CSS Snippet-->
<div class="puik-search">
<div class="puik-search__wrapper puik-search__wrapper--rounded-input">
<input class="puik-search__field" type="text" rounded :autocomplete="'test'"/>
</div>
</div>
<!--VueJS Snippet-->
<puik-search v-model="myValue" rounded autocomplete="search"/>
<!--HTML/CSS Snippet-->
<div class="puik-search">
<div class="puik-search__wrapper puik-search__wrapper--rounded-input">
<span class="puik-search__icon puik-h2">search</span>
<input class="puik-search__field" type="text" autocomplete="search" />
</div>
</div>

The icon is missing & use search as autocomplete value,

packages/components/search/stories/search.stories.ts Outdated Show resolved Hide resolved
@kktos
Copy link

kktos commented May 4, 2023

just a thought but what about a generic way for this ?
e.g:
enhanced the input text component so it can display icons in its zone. On the left or on the right.
With the ability to bind an action.
Then, the search component will be simply a use of this input text component with one icon (x) on the right side with a "clear text" action.

@kktos
Copy link

kktos commented May 4, 2023

We have some searchbox already in our code that's why I'm pretty interested by this component ;)
Kudos for your work.

@FrancoisQtr FrancoisQtr dismissed stale reviews from aAmorim27 and ga-devfront via ba07960 May 4, 2023 13:53
@mattgoud
Copy link
Collaborator

outdated (based on v1)

@mattgoud mattgoud closed this Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Type: New Feature VueJS Tech: VueJS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Component] Search
5 participants