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

Fixes #1807: [UI 2.0] SearchHelp #1909

Merged
merged 1 commit into from
Mar 24, 2021
Merged

Conversation

rjayroso
Copy link
Contributor

Issue This PR Addresses

Implements and closes #1807

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

As per the new 2.0 UI Design, I have re-styled the search helper. See pictures below:

Web
Capture2
Web Design Reference
Mobile
Capture1
Mobile Design Reference

See #1804 for complete design reference. Currently, the SearchHelper component is static - I think that it should be handled within the SearchBar/SearchInput components (when the user types the help disappears). Let me know if this isn't the case cc @PedroFonsecaDEV

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)

@izhuravlev
Copy link
Contributor

@rjayroso I launched the vercel deployment, and after making a request saw this:
image
I don't think this should work like this

Copy link
Contributor

@izhuravlev izhuravlev left a comment

Choose a reason for hiding this comment

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

Looks good except for the display error I specified above - try looking into web/src/pages/search.tsx to solve it.

Also, it will be great if you can squash those commits when the PR is ready.

maxWidth: 500,
fontSize: theme.typography.pxToRem(22.5),
border: '1px solid #dadde9',
subtitle: {
Copy link
Contributor

@izhuravlev izhuravlev Mar 12, 2021

Choose a reason for hiding this comment

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

I don't see where are you using this Subtitle - do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! It's not needed anymore I'll take it out after I figure out how to work out that display error you found above

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, ping me if you actually need help figuring it out

const useStyles = makeStyles((theme: Theme) => ({
container: {
width: '50%',
transform: 'translate(-50%, -50%)',
Copy link
Contributor

@izhuravlev izhuravlev Mar 12, 2021

Choose a reason for hiding this comment

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

Why do you need to transform?

src/web/src/components/SearchHelp.tsx Outdated Show resolved Hide resolved
@rjayroso rjayroso changed the title Issue 1807 Fixes: Issue 1807 Mar 12, 2021
@rjayroso rjayroso changed the title Fixes: Issue 1807 Fixes: Issue 1807 [UI 2.0] SearchHelp Mar 12, 2021
@rjayroso rjayroso changed the title Fixes: Issue 1807 [UI 2.0] SearchHelp Fixes #1807: [UI 2.0] SearchHelp Mar 12, 2021
humphd
humphd previously requested changes Mar 12, 2021
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 isn't using the right font (Times New Roman), needs to be removed when the loading spinner starts, and when search content is shown. I'm also not a huge fan of centered text like this.

@PedroFonsecaDEV
Copy link
Contributor

PedroFonsecaDEV commented Mar 16, 2021

@rjayroso Rebase

@izhuravlev What do you think about it?

@huynguyez I think your new design will land after this PR. So I would appreciate your opinion here.

I think this PR will be a temporary solution to the component.

@rjayroso
Copy link
Contributor Author

rjayroso commented Mar 16, 2021

@rjayroso Rebase

@izhuravlev What do you think about it?

@huynguyez I think your new design will land after this PR. So I would appreciate your opinion here.

I think this PR will be a temporary solution to the component.

Sounds good to me! In the meantime, I tried my best to address the issues above.

  1. SearchHelp now disappears on submit
  2. Font changed to TimesNewRoman
  3. Now aligned to the left and uses standard CSS attributes

Excuse the old search bar below, I haven't pulled the current design yet

8c4b04f80c2bfacc8ce43fbc8843cc13.mp4

mobile

Let me know if I need to change anything, font colour, size, etc.

@rjayroso rjayroso requested review from izhuravlev and humphd March 16, 2021 12:29
@izhuravlev
Copy link
Contributor

@rjayroso I love it, looks good to me. The only thing that I thought of is that we can probably surround it with a separate "Box". What I mean is that the SearchHelp can be displayed in a rectangle of a different colour, so it is visually separated - but this smth we should ask @PedroFonsecaDEV about.

@izhuravlev
Copy link
Contributor

izhuravlev commented Mar 16, 2021

@rjayroso Okay, I just opened the deployment and this is what I saw:
image

After searching it removes the bottom SearchHelper, but it leaves the top one be.

Also, not sure if it is related, on small resolutions like iPhone X if no results have been found only the picture is displayed - I can't scroll to the "no Results Found" Message.
image

Try rebasing it and see if it changes anything. You might also look into the <SearchBar> as From what I see, I believe that the <SearchHelp> text might be getting pasted after the Search heading somehow.

image

@rjayroso rjayroso added this to the 1.9 Release milestone Mar 16, 2021
Copy link
Contributor

@PedroFonsecaDEV PedroFonsecaDEV left a comment

Choose a reason for hiding this comment

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

Rebase so we can review it with the latest version of our master.

@izhuravlev
Copy link
Contributor

So far this looks pretty good. If we don't want to play with the logic a bit and make this being displayed only when the search page is shown for the first time - LGTM. other than that - I would play a bit with the logic here, so the SearchHelp is displayed when there are no results found as well.

@rjayroso
Copy link
Contributor Author

So far this looks pretty good. If we don't want to play with the logic a bit and make this being displayed only when the search page is shown for the first time - LGTM. other than that - I would play a bit with the logic here, so the SearchHelp is displayed when there are no results found as well.

I think we can merge this and I'll file an issue to improve the SearchHelp logic

izhuravlev
izhuravlev previously approved these changes Mar 19, 2021
Copy link
Contributor

@izhuravlev izhuravlev left a comment

Choose a reason for hiding this comment

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

LGTM, Don't forget to file a new issue.

@HyperTHD HyperTHD self-requested a review March 23, 2021 14:35
HyperTHD
HyperTHD previously approved these changes Mar 23, 2021
Copy link
Contributor

@HyperTHD HyperTHD left a comment

Choose a reason for hiding this comment

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

Rebase this, and you can merge this in @rjayroso

@HyperTHD HyperTHD dismissed stale reviews from PedroFonsecaDEV and humphd March 23, 2021 17:04

Changes were made

* Removed HTML Tooltip, added className to elements
* Cleaned up header imports
* Removed old CSS and implemented new UI styling
* Changed CSS values to better fit with planned design
* Removed subtitle styling, no longer needed
* Added state hook to hide visibility of SearchHelp
* Redesign implemented
* Rebased and put SearchHelp below SearchResults in layout
@rjayroso rjayroso requested review from HyperTHD and izhuravlev March 24, 2021 20:09
@rjayroso
Copy link
Contributor Author

Rebased

@rjayroso rjayroso merged commit 5e45d40 into Seneca-CDOT:master Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: front-end area: nextjs Nextjs related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI 2.0] Search Page - SearchHelper
7 participants