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

[WIP]change in Search bar on focus #155

Closed
wants to merge 8 commits into from
Closed

[WIP]change in Search bar on focus #155

wants to merge 8 commits into from

Conversation

raashika03
Copy link
Contributor

@raashika03 raashika03 commented Apr 28, 2021

Checklist

  • I indicated which issue (if any) is closed with this PR using closing keywords
  • I only changed lines related to my PR (no bulk reformating)
  • I indicated the source and check the license if I re-use code, or I did not re-use code
  • I made my best to solve only one issue in this PR, or explain why multi had to be solved at once.

Issue

resolve #102 was working for this issue but then found few more improvements which could've been done, so covered all those in this PR. Later on, I can remove them if not needed.

Details

Four changes in this PR:

  1. Changed the position of search-icon from right to left, as when search icon is on the right side it gives a wrong impression to the user that it's clickable.
  2. Activated the cancel button which is there in browser like chrome, IE but has been disabled because of bootstrap import.
    There's no cancel button in mozilla-firefox by default. It can be included dynamically. Didn't cover for mozilla yet, didn't feel the requirement. For others I thought if it's there by default then it should be active.
  3. provided a transition effect to the search bar, so that it can acquire less space on the header when not in use. And expanded it more for focused state for full desktop size(which is used mostly) for better visualisation.
  4. Changed the position of Dark-theme button.
    Almost all of these changes are related to each other and smaller, so covered them in the same PR.
Search-bar.mp4

@bryan-brancotte @matuskalas @hmenager please review!

Copy link
Member

@bryan-brancotte bryan-brancotte left a comment

Choose a reason for hiding this comment

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

Hi,
You kind of try to fix two issue in one PR: #102 as stated, but also the location of the theme button (I now created an issue for it: #159). Please do one PR per issue/functionality

@raashika03
Copy link
Contributor Author

Done @bryan-brancotte
Now could you review for search bar only.
sorry for the inconvenience, I thought these are interconnected a bit so covered them in the same PR.
Sorry again. :(

Copy link
Member

@bryan-brancotte bryan-brancotte left a comment

Choose a reason for hiding this comment

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

Hi,

I havn't understood why such refactoring, what we have for now is fine isn't it ? Adding such expanding effect might look nice to some people, but other don't (:raising_hand_man:). In addition with such effect there is always a risk of unexpected side effect, and a long time used at fixing it. Here is one of the current side effect.

move

So, In a new PR, changing type="text" to type="search" yes, it is sound, but the rest I vote no, sorry.

@raashika03
Copy link
Contributor Author

raashika03 commented Apr 29, 2021

  1. Changed the position of search-icon from right to left, as when search icon is on the right side it gives a wrong impression to the user that it's clickable.
  2. Activated the cancel button which is there in browser like chrome, IE but has been disabled because of bootstrap import.
    There's no cancel button in mozilla-firefox by default. It can be included dynamically. Didn't cover for mozilla yet, didn't feel the requirement. For others I thought if it's there by default then it should be active.

So, I can go with these changes right? @bryan-brancotte in other PR

but the rest I vote no, sorry.

HAHA still thank you. May be you're right cause that transition is quite frequent which might not look good. I thought to increase the search bar size for better focus on the autocomplete, But your point is valid, I agree.
PS- that transition effect became so fast because of removal of theme button 😅

@raashika03
Copy link
Contributor Author

I'll see some other elegant approach for #102 to restrict the change in visual window. thank you :)

@raashika03 raashika03 changed the title change in Search bar on focus [WIP]change in Search bar on focus Apr 29, 2021
@@ -38,7 +38,22 @@
.flex-column{
flex-direction: column;
}

input[type="search"]::-webkit-search-cancel-button {
-webkit-appearance: searchfield-cancel-button;
Copy link
Member

Choose a reason for hiding this comment

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

I would rather not use it as https://developer.mozilla.org/fr/docs/Web/CSS/::-webkit-search-cancel-button and also we have to move the glass, I rather stay with standard web stuf when we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok then we can set that cancel button dynamically only, because bootstrap doesn't renders that to be displayed by default. But I guess cancel button is not so useful so, I should leave this.
I looked for many approaches then still bootstrap hasn't fixed it. So should I leave this or create dynamically using JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this discussion bootstrap hides it purposely.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed I wasn't aware of that. So if even Bootstrap says we should not have cancel button, we should not have it.

As a conclusion no css changes, not html changes ? Just close this PR ?

@@ -38,7 +38,22 @@
.flex-column{
flex-direction: column;
}

input[type="search"]::-webkit-search-cancel-button {
-webkit-appearance: searchfield-cancel-button;
Copy link
Member

Choose a reason for hiding this comment

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

Indeed I wasn't aware of that. So if even Bootstrap says we should not have cancel button, we should not have it.

As a conclusion no css changes, not html changes ? Just close this PR ?

@raashika03
Copy link
Contributor Author

raashika03 commented Apr 29, 2021

Changed the position of search-icon from right to left, as when search icon is on the right side it gives a wrong impression to the user that it's clickable.

? If you think this is logical. else I'll close my self :)
style="font-family: FontAwesome, Arial;"

placeholder=" Search for concepts here, enter at least two letters"

@bryan-brancotte
Copy link
Member

(...) when search icon is on the right side it gives a wrong impression to the user that it's clickable.

I agree with you, but at the moment user enters a second letter the autocomplete is triggered and user knows that search have already been done

@raashika03
Copy link
Contributor Author

Oki then I'm closing it, thank you for the discussion :) @bryan-brancotte

@raashika03 raashika03 closed this Apr 29, 2021
@raashika03 raashika03 deleted the Search-bar branch May 2, 2021 15:35
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.

Overflow of search bar in smart phones
2 participants