- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.8k
feat: Add images filter in explore page #5923
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 images filter in explore page #5923
Conversation
| This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/hi2i42trb | 
| Codecov Report
 @@               Coverage Diff               @@
##           development    #5923      +/-   ##
===============================================
- Coverage        23.53%   23.50%   -0.04%     
===============================================
  Files              511      511              
  Lines             5489     5501      +12     
  Branches            66       66              
===============================================
+ Hits              1292     1293       +1     
- Misses            4180     4191      +11     
  Partials            17       17              
 Continue to review full report at Codecov. 
 | 
| Change to has_image, has_logo | 
        
          
                app/components/explore/side-bar.js
              
                Outdated
          
        
      | eventLocationType = null; | ||
|  | ||
| @computed('category', 'sub_category', 'event_type', 'startDate', 'endDate', 'location', 'ticket_type', 'cfs', 'event_name', 'is_online') | ||
| @computed('category', 'sub_category', 'event_type', 'startDate', 'endDate', 'location', 'ticket_type', 'cfs', 'event_name', 'is_online', 'has_logo', 'has_images') | 
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.
has_image, not has_images
        
          
                app/components/explore/side-bar.js
              
                Outdated
          
        
      | event_name : null, | ||
| is_online : null, | ||
| has_logo : 'false', | ||
| has_image : 'false', | 
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 believe this has wrong indentation? Please check, I'm reviewing on Mobile
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.
ohh sorry. i give replace all command. thats why identation got wrong. sorry
i will fix this
| On mobile, all filters except CFS and image options are hidden. Please hide them as well. And the filters are pre-selected. Please unselect | 
| @iamareebjamal plz review | 
        
          
                app/routes/explore.js
              
                Outdated
          
        
      | if (!params.has_image) { | ||
| filterOptions.push({ | ||
| name : 'original-image-url', | ||
| op : 'ne', | ||
| val : null | ||
| }); | 
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.
the condition used inside is for checking if it has a image. And it is named like "not has_image".
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.
When has_image=null, this condition become true and only events with images are shown.
When has_image=false, this condition hold false and all events whether image is present or not will be shown.
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.
That's opposite logic though. has_image=true should only show events with images
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.
That's opposite logic though. has_image=true should only show events with images
Now i am little bit confused.
Please correct me if i am wrong.
Desired result:----> when has_image=null, all events shown whether it have image or not.
When has_image=true, events with images must be shown.
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.
Yes
| op : 'ne', | ||
| val : null | ||
| }); | ||
| } | 
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.
same here
f4008d6    to
    2a7b29f      
    Compare
  
    
Fixes #5756
Short description of what this resolves:
add images filter
Checklist
developmentbranch.