#245 Improve side navigation design
Merged by jflory7. Opened by alishapapun.
fedora-commops/ alishapapun/fedora-happiness-packets navigation-test  into  master

Download 245.patch

This PR addresses #184 and includes search functionality in the archive by merging archive page and search page.

Metadata Update from @jflory7:
- Pull-request tagged with: PASSED, improvement, needs testing, type - backend, type - frontend, type - summer coding

Metadata Update from @jflory7:
- Request assigned

Could id="search_fasid" be removed since it was removed from custom.css?

Maybe we could put this above the _message_list so it is easier to see. What do you think?

Could you please choose a more a descriptive name for this CSS ID? When I read through the CSS file, it's not obvious #id_q is for the archive search bar. It should be clear what the style elements are for from the name.

Since this is the landing page and the first page the user will visit when opening the site, it might make more sense to move it to the top of the sidebar menu. What do you think?

Hi @alishapapun! The sidebar looks way cleaner and less distracting. It's also cool to have the search and archive pages combined! :raised_hands: Nice work on figuring this one out since it was trickier than I originally expected.

I came across one problem while testing. I wasn't able to return any messages from the search bar. I tried using names, email addresses, message identifiers, and text from public messages, but it always returned empty. Were you able to get messages to return when you tried a search?

Otherwise, I left minor feedback as in-line comments above. I also suggest pulling changes from master branch and rebasing your branch on top of changes since some other PRs were merged in the meanwhile.

Do you think you can make these changes by Wednesday? Let me know once you have a chance to look through the feedback. :) Thanks for sticking through with this one.

Metadata Update from @jflory7:
- Pull-request untagged with: needs testing
- Pull-request tagged with: needs changes

Metadata Update from @jflory7:
- Request assigned

Could id="search_fasid" be removed since it was removed from custom.css?

No, Justin. This ID is used by fas_details.js to fetch the value entered by the user to search the username, it can't be removed.

Maybe we could put this above the _message_list so it is easier to see. What do you think?

Sure! I will make the change.

Could you please choose a more a descriptive name for this CSS ID? When I read through the CSS file, it's not obvious #id_q is for the archive search bar. It should be clear what the style elements are for from the name.

The search box present in the search.html is an element of haystack.forms and the id generated by is automatically generated. So the ID of the search element in archive.html has to be the same as that of search.html since it requires the same value. Am I making sense?

Since this is the landing page and the first page the user will visit when opening the site, it might make more sense to move it to the top of the sidebar menu. What do you think?

Sure! I will rename it as Home then.

2 new commits added

  • Merge archive and search page
  • Improve side navigation design

rebased onto fe3f5da5d310bf67f3503126ebb259af58d03625

Hi @alishapapun! The sidebar looks way cleaner and less distracting. It's also cool to have the search and archive pages combined! 🙌 Nice work on figuring this one out since it was trickier than I originally expected.
I came across one problem while testing. I wasn't able to return any messages from the search bar. I tried using names, email addresses, message identifiers, and text from public messages, but it always returned empty. Were you able to get messages to return when you tried a search?
Otherwise, I left minor feedback as in-line comments above. I also suggest pulling changes from master branch and rebasing your branch on top of changes since some other PRs were merged in the meanwhile.
Do you think you can make these changes by Wednesday? Let me know once you have a chance to look through the feedback. :) Thanks for sticking through with this one.

Hey, @jflory7 I made the required changes and tested. It's working perfectly. I also rebased onto the top of changes. When you get some time, do check out this PR. Thanks!

Metadata Update from @jflory7:
- Pull-request untagged with: needs changes
- Request assigned

The search box present in the search.html is an element of haystack.forms and the id generated by is automatically generated. So the ID of the search element in archive.html has to be the same as that of search.html since it requires the same value. Am I making sense?

I didn't know it was automatically generated. Thanks for pointing it out.

Hey, @jflory7 I made the required changes and tested. It's working perfectly. I also rebased onto the top of changes. When you get some time, do check out this PR. Thanks!

Awesome, everything works as expected on my end! Nice work @alishapapun. :tada: I'm going to merge this one now! :ocean:

Pull-Request has been merged by jflory7