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

Issue 1224 Reposition search button for search UI #1381

Closed
wants to merge 10 commits into from
Closed

Issue 1224 Reposition search button for search UI #1381

wants to merge 10 commits into from

Conversation

strawberries73
Copy link
Contributor

@strawberries73 strawberries73 commented Nov 17, 2020

Issue This PR Addresses

Fixes #1224

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Moved the Icon search button (magnifying glass) down into the right of the input text box. Works for all devices.

image

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@agarcia-caicedo
Copy link
Contributor

This is a small nit, but can you center it more between the end of the text box and the edge of the container?

@strawberries73
Copy link
Contributor Author

@agarcia-caicedo I will work on that.

@c3ho
Copy link
Contributor

c3ho commented Nov 17, 2020

Can we edit the title of this PR?

@strawberries73 strawberries73 changed the title Issue 1224 Issue 1224 Moved search Icon to align with input text box Nov 17, 2020
@strawberries73 strawberries73 changed the title Issue 1224 Moved search Icon to align with input text box Issue 1224 Reposition search button for search UI Nov 17, 2020
@strawberries73
Copy link
Contributor Author

@agarcia-caicedo I centered the icon button.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is working well at large widths, but fails at narrower widths and on mobile.

search-button-overlap

@strawberries73
Copy link
Contributor Author

@humphd I am trying to figure this one out.

@strawberries73
Copy link
Contributor Author

@humphd fulfilled request, it now works on small windows.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is a lot better. There's still an issue on mobile, though:

Screen Shot 2020-11-18 at 9 33 06 AM

@strawberries73
Copy link
Contributor Author

strawberries73 commented Nov 18, 2020

I notice this too.
image
The problem is that I do not know how to do this part. I need help with this.
image

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

It's getting cropped on mobile, you need to allow a more flexible size on the input so it makes room vs. pushing it off the screen.

Screen Shot 2020-11-18 at 2 33 07 PM

@agarcia-caicedo
Copy link
Contributor

A suggestion, but in mobile screens, rather than have the filter and the search input take up two lines, like it currently does, we can shrink the filter so it takes up less space and have both the filter and search in one line.

That way there could be space for the search button

@huyxgit
Copy link

huyxgit commented Nov 19, 2020

@agarcia-caicedo @humphd @strawberries73 I tried to use Input Adornments for the Textfield and It works pretty nice. I prefer its default (light grey) color than blue for the search icon.

@strawberries73
Copy link
Contributor Author

@VIETNAMEZE I like it personally. How does it look on mobile?

@huyxgit
Copy link

huyxgit commented Nov 19, 2020

@VIETNAMEZE I like it personally. How does it look on mobile?

@strawberries73 Same as the gif above

@agarcia-caicedo
Copy link
Contributor

@VIETNAMEZE I think that's a viable solution. I'm not a fan of how the button looks with the fix, but that could be a follow up issue, since it's more of a purely aesthetic nit

@huyxgit
Copy link

huyxgit commented Nov 19, 2020

@VIETNAMEZE I think that's a viable solution. I'm not a fan of how the button looks with the fix, but that could be a follow up issue, since it's more of a purely aesthetic nit

@agarcia-caicedo That's why I prefered the search icon to be in grey color, the blue doesn't look nice. it doesn't need to be stand out too much since people more likely to hit 'enter' than press the button

@agarcia-caicedo
Copy link
Contributor

agarcia-caicedo commented Nov 19, 2020

@VIETNAMEZE I think that's a viable solution. I'm not a fan of how the button looks with the fix, but that could be a follow up issue, since it's more of a purely aesthetic nit

@agarcia-caicedo That's why I prefered the search icon to be in grey color, the blue doesn't look nice. it doesn't need to be stand out too much since people more likely to hit 'enter' than press the button

I do remember @humphd wanting a button that looks like a button, so that's why I'm hesitate to make it less noticeable and more like a decoration

@strawberries73
Copy link
Contributor Author

@agarcia-caicedo I was able to put the changes in that @VIETNAMEZE suggested. Personally I like the blue search button. :)

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

OK, I think this direction will work. In a follow-up we can revisit the actual look and feel, I'm not sure the blue is ideal here, but it doesn't need to block things.

However, your branch management has undone a number of things on master when you did Merge branch 'master' into Issue-1224 with commit f291f8d. For example, clicking the adornment no longer does a search, and we get errors in the console. Please make sure you re-read the git workflow document I linked on Blackboard: https://github.com/Seneca-CDOT/telescope/blob/master/docs/git-workflow.md.

If you're able to rebase and correct these merge conflicts, that would be good. If not, please close this and open a new PR that only changes the code related to the new <InputAdornment>.

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.

Show all posts for an author sorted by date when searching by Author
5 participants