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

Feat/add filter moments page #2011

Merged
merged 12 commits into from
Feb 20, 2024
Merged

Feat/add filter moments page #2011

merged 12 commits into from
Feb 20, 2024

Conversation

franpb14
Copy link
Contributor

@franpb14 franpb14 commented Sep 11, 2021

Description

Hello! I have made a new Input type with React ("multi_select") and I have added this with some filters to the moments.

More Details

React's component: I have created it looking a little bit at the implemented inputs and without using any new library. I have also added this component to the storybook and I have also added some tests.

Backend: I have added code to existing functions and added some methods to the helper. I have tried to make the code as reusable as possible in case, at some point, you want to do the same for other sections. I have also implemented new tests.

Note: there are some of the filters that are opposite so no moment is returned.
Note 2: I have corrected some messages in Spanish (my native language).
Last note: This is my first pull request with real code here, there are probably many things to correct, thanks to my reviewer in advance!

Corresponding Issue

#1994

Screenshots

Here is a working video:

Issue1994.mp4

And here is a screenshot of the mobile view

Screenshot from 2021-09-11 12-49-30


Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

@julianguyen julianguyen self-requested a review September 12, 2021 16:56
@julianguyen
Copy link
Member

Sorry I haven't had a chance to take a look at this! Will do soon!

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Thanks so much for working on this! Sorry for the late review, it's been a busy month for me.

Let me know if you have any questions!

The other thing I noticed is if a user just has one Moment and the filter results in zero results, we display our experience when there are no moments:

image

It makes sense why we are showing this view, but I think it can be potentially confusing to users. We might actually just want to show all Moments when no filters match and indicate that no filters match.

app/controllers/concerns/collection_page_setup_concern.rb Outdated Show resolved Hide resolved
app/controllers/search_controller.rb Outdated Show resolved Hide resolved
client/app/components/Input/InputMultiSelect.jsx Outdated Show resolved Hide resolved
client/app/components/Input/InputMultiSelect.jsx Outdated Show resolved Hide resolved
client/app/components/Input/InputMultiSelect.jsx Outdated Show resolved Hide resolved
client/app/components/Input/InputMultiSelect.scss Outdated Show resolved Hide resolved
@julianguyen julianguyen self-assigned this Feb 19, 2024
julianguyen and others added 3 commits February 19, 2024 12:08
@julianguyen
Copy link
Member

I'm going to finish up this PR so we can merge it! :)

I've added a new jsx which tests the new component and also I've modified some test for not repeat code.
… storybook

I have also deleted some of my unneeded code #1994
@julianguyen julianguyen force-pushed the feat/add-filter-moments-page branch from c15b9a3 to d985808 Compare February 19, 2024 20:15
@julianguyen julianguyen force-pushed the feat/add-filter-moments-page branch 7 times, most recently from 4e5e500 to d012f3a Compare February 20, 2024 04:20
@julianguyen julianguyen force-pushed the feat/add-filter-moments-page branch from d012f3a to 4c4cd7a Compare February 20, 2024 06:11
@julianguyen julianguyen merged commit 5d61d3c into main Feb 20, 2024
6 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feat/add-filter-moments-page branch February 20, 2024 06:31
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.

2 participants