-
Notifications
You must be signed in to change notification settings - Fork 3
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
Show hidden offers to admin #291
base: develop
Are you sure you want to change the base?
Conversation
@CarlosMealha @diogofonte any updates on this? |
We were waiting for opinions about the divs that are causing extra space in mobile view search, but the button "show hidden offers" is ready. |
Also, is there any endpoint responsible to fetch hidden offers or we need to filter the offers in order to specify them ? |
the If you are asking in the context of this PR, the feature that needs to be implemented is the possibility for admins to pass that |
I think I looked around a while ago and recall this being an issue in the MultiOptionAutocomplete component, not that one, but i might be wrong. If the problem is in the component you showed maybe you could replace the React Fragment (the |
Yes, my question was about the endpoint specifically, and your answer cleared my doubts. Thank you! |
Okay, me and Diogo made the show hidden toggle on admin account working properly |
a8d6e06
to
04a1c18
Compare
Since there is no backend to test this (at the moment), please include a small video demonstrating this feature. |
ed77758
to
598f1ae
Compare
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.
There should be tests regarding the UI and Request making parts of this feature, not just the Redux store one.
Also, beware of the upstream changes, you'll need to rebase this branch with develop later. |
726749a
to
bcb8ca8
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #291 +/- ##
===========================================
- Coverage 89.05% 88.84% -0.21%
===========================================
Files 176 175 -1
Lines 3390 3345 -45
Branches 851 845 -6
===========================================
- Hits 3019 2972 -47
- Misses 371 373 +2
☔ View full report in Codecov by Sentry. |
6e15449
to
695e803
Compare
We will take care of this as the feature for the related showHidden URL filter are still in progress and the corresponding tests |
be605e5
to
d2acc37
Compare
b3433d2
to
140ea74
Compare
Now the showHidden attribute is present in the URL Video demonstrating the final result: show_hidden_offers.mp4 |
I'll leave the code review for the others but I think the result looks neat! |
b91bae4
to
b3094dc
Compare
Co-authored by: CarlosMealha up202005954@edu.fc.up.pt
…ns inside of the advancedSearchContainer grid
Co-authored by: CarlosMealha up202005954@edu.fc.up.pt
Co-authored-by: diogofonte <diogo.fonte2000@gmail.com>
…s-fe into feature/showHiddenToAdmin
210f355
to
1d046e2
Compare
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.
Everything looks good and is working.
There is just a small detail that in terms of functionality doesn't really matter probably, but basically, if a user without privileges types showHidden=false or true, it shows a dot on the settings icon near the search bar, but then when we click it, in the advanced filters menu, there is obviously nothing there.
However, I believe this doesn't do anything because as far as I saw, the backend checks if a user has privileges or not to show the hidden offer and his an edge case that would probably never happen, so it's not that important.
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.
Left a small comment.
@DoStini please review this code once more, since Github is picky about change requests 🙃
gridRowStart: 4, | ||
gridColumnStart: 2, | ||
}, | ||
spaceBtwn: { |
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'm not a big fan of this: this class should have another name, and not one that simply re-states what it does style-wise.
Also, don't forget to rebase. |
Closes #236