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

Including classes on searchBar to identify when is open and/or filled #792

Merged
merged 6 commits into from
Jun 17, 2020

Conversation

igorpoubel
Copy link
Contributor

What problem is this solving?

It includes classes to check if the result list is opened and if the input is filled

How should this be manually tested?

Checking if the class "open" will be included when starts to fill the input. And check if the class "hasTerm" is included when there is anything on input.

Workspace -> search for file

Checklist/Reminders

  • Updated README.md.
  • Updated CHANGELOG.md.
  • Linked this PR to a Clubhouse story (if applicable).
  • Updated/created tests (important for bug fixes).
  • Deleted the workspace after merging this PR (if applicable).

Screenshots or example usage

When just filled (without focus)
image
image

When opened (focus)
image
image

Type of changes

✔️ Type of Change
_ Bug fix
✔️ New feature
_ Breaking change
_ Technical improvements

Notes

@igorpoubel igorpoubel requested a review from a team as a code owner June 10, 2020 22:07
@vtex-io-ci-cd
Copy link
Contributor

vtex-io-ci-cd bot commented Jun 10, 2020

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch

  • Minor

  • Major

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@vtex-io-docs-bot
Copy link

vtex-io-docs-bot bot commented Jun 10, 2020

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

  • There's nothing new to document 🤔
  • I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

  • I just updated 🎉🎉

Copy link
Contributor

@Klynger Klynger left a comment

Choose a reason for hiding this comment

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

Hi @igorpoubel , thank you for the PR, just a couple of simples changes:

  1. You forgot to add the CHANGELOG
  2. I think in this case you don't have to create new handles, just use applyModifiers, here is an example
  3. Instead of hasTerm I think it would be better filled and opened as a modifier

@Klynger Klynger requested a review from victorhmp June 12, 2020 13:35
@igorpoubel
Copy link
Contributor Author

@Klynger thanks for the notice

on CHANGELOG I'll have to put a new manifest version (even not released) ou I'll just insert on the current version?

For exemplo:
The current version is 3.117.2.

Already have this CHANGELOG:
image

option 1 - So I'll have to insert this way?
image

option 2 - Or this way? (assuming a new release)
image

I think is the option 1, am I right?

@Klynger
Copy link
Contributor

Klynger commented Jun 12, 2020

@Klynger thanks for the notice

on CHANGELOG I'll have to put a new manifest version (even not released) ou I'll just insert on the current version?

For exemplo:
The current version is 3.117.2.

Already have this CHANGELOG:
image

option 1 - So I'll have to insert this way?
image

option 2 - Or this way? (assuming a new release)
image

I think is the option 1, am I right?

No, you just add on Unreleased and don't change anything in manifest.json or package.json, this is done automatically when we launch the version
image

@igorpoubel
Copy link
Contributor Author

igorpoubel commented Jun 12, 2020

@Klynger so I just have to do this?

image

And the version and date will be added automatically. Right?
Do I keep add or fix?

@Klynger
Copy link
Contributor

Klynger commented Jun 12, 2020

@Klynger so I just have to do this?

image

And the version and date will be added automatically. Right?
Do I keep add or fix?

You use Added and remove the empty space between Unreleased and Added. We follow the spec of CHANGELOG, the only difference is the empty line between the version and the sections that we remove

@igorpoubel
Copy link
Contributor Author

@Klynger ohh, now I got it.

Ok! Thanks.

react/components/SearchBar/components/SearchBar.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Klynger
Copy link
Contributor

Klynger commented Jun 15, 2020

Hi @igorpoubel , after you make the changes you can request my review again so I can get notified to review your PR again

igorpoubel and others added 3 commits June 16, 2020 16:57
Co-authored-by: Rafael Klynger <rafael.klynger@gmail.com>
Co-authored-by: Rafael Klynger <rafael.klynger@gmail.com>
Co-authored-by: Rafael Klynger <rafael.klynger@gmail.com>
@igorpoubel igorpoubel requested a review from Klynger June 16, 2020 20:01
@@ -186,7 +186,7 @@ const SearchBar = ({
<div
className={classNames(
'relative-m w-100',
handles.searchBarInnerContainer
applyModifiers(handles.searchBarInnerContainer, [isOpen ? 'opened' : '', inputValue ? 'filled' : '']),
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some lint problems, you can fix this by installing the dependencies from the root of the app and running yarn lint --fix

@igorpoubel igorpoubel requested a review from Klynger June 17, 2020 01:10
@vtex-io-docs-bot
Copy link

Beep boop 🤖 That's ok, I created an issue for this so we don't forget

@Klynger
Copy link
Contributor

Klynger commented Jun 17, 2020

@all-contributors add @igorpoubel for code

@allcontributors
Copy link
Contributor

@Klynger

I've put up a pull request to add @igorpoubel! 🎉

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.

None yet

3 participants