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

refactor: old array conversion #712

Merged
merged 4 commits into from
Sep 28, 2023
Merged

refactor: old array conversion #712

merged 4 commits into from
Sep 28, 2023

Conversation

0o001
Copy link
Contributor

@0o001 0o001 commented Sep 24, 2023

@muratcorlu muratcorlu requested review from a team, fatihhayri and DamlaDemir and removed request for a team September 25, 2023 07:48
src/components/input/bl-input.ts Outdated Show resolved Hide resolved
@AykutSarac
Copy link
Member

AykutSarac commented Sep 25, 2023

@muratcorlu Are we sure that we want to support the search input type that way? We haven't discussed the UI/UX decisions of this component. I'd say avoid the effort before the discussion.

image

@muratcorlu
Copy link
Contributor

@AykutSarac Good question. I'm not sure. What kind of consequences do you predict? Do you mean considering to use a relevant icon while we use search input?

@0o001
Copy link
Contributor Author

0o001 commented Sep 25, 2023

There are two CSS tags we can use for "search" One is for the search input, and the other is for the search results. Additionally, if this matter is important, you might consider evaluating this situation for other types as well.

https://developer.mozilla.org/en-US/docs/Web/CSS/::-webkit-search-cancel-button
https://developer.mozilla.org/en-US/docs/Web/CSS/::-webkit-search-results-button

Like this;
https://developer.mozilla.org/en-US/docs/Web/CSS/::-webkit-inner-spin-button

Others;
https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements

@AykutSarac
Copy link
Member

@AykutSarac Good question. I'm not sure. What kind of consequences do you predict? Do you mean considering to use a relevant icon while we use search input?

We need to show our own icons, so we have to create the functionality for displaying and clearing them. However, this component might be more complicated than we think. It's important to decide what we expect from a search input, as new modifications later could cause problems with the architecture.

image

@muratcorlu
Copy link
Contributor

MDN says:

<input> elements of type search are text fields designed for the user to enter search queries into. These are functionally identical to text inputs, but may be styled differently by the user agent.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/search

Currently we use text type with search icon to show a search input:

<bl-input type="text" icon="search" ...></bl-input>

Since technically there is no behavioral diffierence between text and search types, we may only consider to set a default icon per type (also for datetime, for example) to provide a better consistency throughout all projects that uses Baklava.

<bl-input type="search"></bl-input>  <!-- comes with search icon by default -->

But, with the multiple icon registry support, this may be more confusing and tricky. Because search icon can be named differently in other icon libraries. (It's magnifying-glass on fontawesome icons, for example) But on the other hand, we already use default icons in many other places (info icon on tab component and alert component, exclamation icon on input error states, etc.) so this needs to be addressed anyway for other places as well.

So, considering we already have datetime type without a default icon applied, I don't see a big difference here. We already planned to have date picker component but in the meantime we thought it's ok to just being able to set datetime type for input to just have native browser UX for now, without waiting to implement date picker. They seem very similar to me.

@AykutSarac
Copy link
Member

#713 will bring the icons for input types so we don't need to cover here. Do we have any TODO(s) for this PR anymore? @muratcorlu

@0o001
Copy link
Contributor Author

0o001 commented Sep 27, 2023

#713 will bring the icons for input types so we don't need to cover here. Do we have any TODO(s) for this PR anymore? @muratcorlu

2056ca3

Perhaps you can consider this part.

@muratcorlu
Copy link
Contributor

muratcorlu commented Sep 28, 2023

Yeah, we can consider this as a refactor proposal. @0o001 If you can rollback the input type addition from this PR, we can consider and merge this separately.

@0o001 0o001 changed the title fix(input): add search input type refactor: old array conversion Sep 28, 2023
@muratcorlu muratcorlu merged commit 813e50f into Trendyol:next Sep 28, 2023
5 checks passed
@muratcorlu
Copy link
Contributor

Thanks for your contribution @0o001 🚀

AykutSarac added a commit that referenced this pull request Sep 29, 2023
I decided that'd be a good idea to display icons depending on the type
of the input.

> `icon` attribute overrides the default icon.

<img width="844" alt="image"
src="https://github.com/Trendyol/baklava/assets/47941171/3025aee8-5357-45c1-946f-01ab7fa5b680">

```html
<bl-input type="search" />
<bl-input type="date" />
<bl-input type="date" icon="academy" />
<bl-input type="time" />
```

Additionally, this PR adds input type of `search`, it could be good idea
to review #712 first.

---------

Co-authored-by: Aykut Saraç <aykut.sarac@trendyol.com>
@github-actions
Copy link

🎉 This PR is included in version 2.3.0-beta.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@AykutSarac AykutSarac mentioned this pull request Oct 12, 2023
@github-actions
Copy link

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants