-
Notifications
You must be signed in to change notification settings - Fork 27
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
feature/events-page #110
feature/events-page #110
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per usual, I have no comments on the code. This all looks great to my naive JS eye. I am sure others will have comments though.
On design aspects, this looks great! I have some nitpicks but I am not sure what others think.... maybe we merge, deploy to seattle staging and then have everyone test it out and give design feedback / nitpicks in slack?
My biggest one is, it's somewhat hard to tell that these are "cards" (that the whole area of the thumbnail + the text area is clickable)... We could put a border around it or just border bottom or something, or we could change the background color of the page or of the cards to be a tiny bit different. I looked at what youtube does for comparison and they have a similar card setup but when you hover a "more options" triple dot dropdown appears. i dont think we have a need for more options personally. although maybe others have some ideas.
Can you swap the text for "Tags" to "Keywords" because those are what they are. I feel like we may add tags in the future that are more topic related and less "keyword" related so good to pre-emptively make that change now.
the keywords also show me that I think the indexer may need to be changed. the keywords definitely help but i think the weighting is a bit off... bi- and tri-grams should carry more weight than unigrams for example and I see a lot of unigrams. Will investigate this weekend hopefully.
On the display of keywords, can we standardize them all to entirely uppercase or lowercase, it looks a bit odd when they are mixed.
We should add a search bar to the top of the page imo. Make it easy to switch from event browsing to searching directly.
👍
https://protocol.mozilla.org/demos/card-layout.html. The Mozilla protocol cards have a border on hover, I don't know why they aren't showing up on hover. I could just add a border (doesn't matter whether it's hover or not)?
Yes
I support either, but I think lowercase is a little better.
To clarify, the search bar would take the user to |
Ah! I've been meaning to make an issue for this for literally months and keep forgetting. When I made the card component a long time ago I think I messed up the implementation and forgot to include the CSS class in charge of rendering the hover border for card components. 😅 I've lost all context at this point so I'm not quite sure what's missing, but I'm sure if you take a look at the docs and visually grep for differences you should hopefully find it. |
It was this class @JacksonMaxfield You OK with a border on the card on hover? |
Thinking about this again. To put a search bar on this page would mean users would expect to be able to use filters in conjunction with the search bar. I'm not sure if I'd want to encode all the filtering info into |
Yep! I think adding the gray border would be good. We can see it in action when we deploy to staging.
Lowercase it is then!
Agree that we don't want to incorporate all the filtering into the URL but couldn't we keep the filtering in state? Route to search, read from state for filters, run the query? Or is that bad? |
It's fine to keep the filtering in state. We will do the same when the user navigates from the home page to Sounds good? |
Yep works for me! |
New commits addressed Jackson's feedback. the search state is
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say this is ready to go. @BrianL3 @hawkticehurst if either of you have time to review the code it would be much appreciated
Yep! I finally have a free moment to give a review now. |
Yay, thank you! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beyond some minor questions and nits looks good!
I also do have some reservations with the usage of lodash
, but I decided it would be better to open a separate issue since we added it in another PR and are using it elsewhere.
Link to Relevant Issue
WIP #57 (the
/events
page)Description of Changes
Network layer
Events page and container
I used the FetchDataContainer to initially fetch all bodies and a batch of events in the EventsPage, which are then provided as props to EventsContainer, where the events are displayed. And where filtering, sorting, and pagination are implemented.
The pagination is sorta like an infinite scroll, where users would see the initial batch of events and they can click
Show more events
to show more events.Misc changes
I modified the Comittee filter UI a bit. Users can only select up to 10 bodies/committees to keep the getEvents function simple. This is because firestore query constraints only allows up to 10 equality in an
OR
query.Changed the trigger buttons of the filters, the clear and save buttons to actual buttons using Mozilla protocol styling.
Link to Forked Storybook Site
I didn't write any stories for this PR.
To verify the changes:
cdp-seattle-staging-dbengvtn
https://user-images.githubusercontent.com/37560480/141205873-186d0374-b163-4a56-8490-ba513b0f05fe.png