Replaced the Placeholder with 'Find' for smaller screen sizes #12997
Conversation
There was a problem hiding this comment.
Thanks for your contribution @Abhishek-Punhani! Code changes makes sese to me, although we have this useKResponsiveWindow composable that can help better in this case :).
Also, just a little note: As flagged in #12238 (comment), we have two things to do here:
* In all cases, truncate "Find something to learn" with ellipsis ...
* Further improve experience by changing "Find something to learn" string to only "Find" on all smaller breakpoints where "Find something to learn" is not visible in its entirety (truncation described above serves rather as fallback to this behavior because we may not be able to estimate precisely)
We have met just the second point with this PR. Lets try to met the first point using some css :).
Let us know if you have any question.
| @@ -117,15 +121,29 @@ | |||
| textAlign: this.isRtl ? 'right' : '', | |||
| }; | |||
| }, | |||
| computedPlaceholder() { | |||
| if (this.viewportWidth < 1264 && this.viewportWidth >= 840) { | |||
There was a problem hiding this comment.
Here instead of checking with these specific pixels, lets instead do this check using our useKResponsiveWindow composable. We can use the windowBreakpoint property, and lets show the "find" string if this windowBreakpoint is 3 or 4 which are the corresponding breakpoints.
| }, | ||
| created() { |
There was a problem hiding this comment.
After using useKResponsiveWindow, we can remove these listeners and watchers since that composable already take care of that.
| @@ -66,6 +66,9 @@ | |||
| export default { | |||
| name: 'SearchBox', | |||
| mixins: [commonCoreStrings], | |||
| $trs: { | |||
There was a problem hiding this comment.
Lets add this property as the last property of the exported object. And lets run yarn lint-frontend:format to correct some linting errors in the PR.
There was a problem hiding this comment.
@AlexVelezLl
Thank you for the feedback and guidance!
I've made the requested changes:
- Implemented the use of useKResponsiveWindow to determine breakpoints dynamically.
- Added a CSS solution to truncate the placeholder text "Find something to learn" with an ellipsis (text-overflow: ellipsis, white-space: nowrap, and overflow: hidden styles applied to the .search-input element).
- Updated the placeholder text to dynamically display "Find" on smaller screens (breakpoint <= 3.5) where truncation isn't sufficient.
- Reduced the placeholder font size for better fit and readability on narrower viewports (font-size: 12px for screens below 1260px).
There was a problem hiding this comment.
Screencast.from.01-14-2025.11.45.39.AM.webm
Abhishek-Punhani
force-pushed
the
Issue#12238
branch
3 times, most recently
from
1e95c8d to
cebbfcf
Compare
There was a problem hiding this comment.
Thanks @Abhishek-Punhani! We are almost there! I just left some minor comments, let me know if you have any question :).
| @@ -117,6 +124,12 @@ | |||
| textAlign: this.isRtl ? 'right' : '', | |||
| }; | |||
| }, | |||
| computedPlaceholder() { | |||
| if (this.windowBreakpoint <= 3.5) { | |||
There was a problem hiding this comment.
We should just test for 3 and 4 breakpoints, so lets update this validation to if ([3, 4].includes(this.windowBreakpoint)) {
| vertical-align: middle; | ||
| border: 0; | ||
| @media (max-width: 1260px) { | ||
| &::placeholder { | ||
| font-size: 12px; |
There was a problem hiding this comment.
Lets remove this font-size, as is being too small, and the font-size of the rest of the text doesnt change, so I think the placeholder text shouldnt neither.
| if (this.windowBreakpoint <= 3.5) { | ||
| return this.$tr('find'); | ||
| } | ||
| return this.placeholder || coreString('searchLabel'); |
There was a problem hiding this comment.
Bear in mind that the this.placeholder value should have max priority. If its defined we should return it regardless of the windowBreakpoint
There was a problem hiding this comment.
@AlexVelezLl Thank you for the feedback!
Just to confirm, should we always display this.placeholder regardless of the screen size? And only when this.placeholder is not defined, we fallback to displaying "Find" for smaller screen sizes? Please let me know if that aligns with the expected behavior, and I can make the necessary adjustments.
There was a problem hiding this comment.
Just to confirm, should we always display this.placeholder regardless of the screen size? And only when this.placeholder is not defined, we fallback to displaying "Find" for smaller screen sizes?
Yes, thats it @Abhishek-Punhani . We only need to tweak the find... string when there isnt a this.placeholder value set
There was a problem hiding this comment.
@AlexVelezLl Thanks for clarifying. I guess I have already pushed required changes .Please let me know if any further changes are required.
Abhishek-Punhani
force-pushed
the
Issue#12238
branch
from
cebbfcf to
5f653d0
Compare
Build Artifacts
|
There was a problem hiding this comment.
Thanks for your contribution @Abhishek-Punhani! Code changes makes sense, and manual QA confirms the expected behaviour. Thanks!
The placeholder text is replaced with a shorter string "Find" when the viewport width is between 840 and 1264 pixels.
Fixes : #12238