PriceRange-PopUp #317
PriceRange-PopUp #317
Conversation
006080
requested review from
grabinskij,
orYoffe,
messkalina,
BajalanIman and
GabrielMelhem
|
Hi, there are some things to improve: Here is one suggested solution among many that worked for me in this project, as I faced the same problem when creating popups. It can be challenging to overlap the header because of its fixed position and placement within the structure. You can resolve this by moving the PriceRangeModal to the App.jsx file, along with the associated styles and states. Additionaly return Header back to z-index:1000. This approach should work 100%.
|
|
By the way, the content inside the popup looks great 👍 |
006080
requested review from
xaved88 and
alinaincodeland
and removed request for
orYoffe,
grabinskij,
GabrielMelhem and
BajalanIman
|
I'm glad you find it great! The small bug you identified in the header can be addressed in a separate ticket, as it’s unrelated to the filter this ticket focuses on. This ticket was created specifically to ensure we didn't lose the work contributed by one of our team members, Kalina, whose efforts have made the main page look much more appealing. |
orYoffe
requested review from
grabinskij,
Konzisam,
GabrielMelhem and
BajalanIman
This PR introduces a new feature but also breaks existing functionality(Category Tabs overlaps dropdowns in the Header). I’m not sure it’s a good practice to merge a feature that negatively impacts other working components. Would it be better to refine this PR to ensure it doesn’t affect other UI elements before merging? This way, we can avoid introducing bugs to the Home page, especially since fixing it before Demo Day might not be possible. What do you think about it? In my previous message, I suggested one possible solution that could resolve the issue. |
|
I created this Component, but unfortunately, I no longer have time to work
on fixing the issues as I have started attending a new school, as I
mentioned before. If you believe it’s better not to include this branch,
which contains a new component also present in the mobile version, that’s
entirely up to you and the other team members. You can proceed in whatever
way you find appropriate.
…On Tue 3. Dec 2024 at 22:20, Bohdan Hrabynskyi ***@***.***> wrote:
I'm glad you find it great! The small bug you identified in the header can
be addressed in a separate ticket, as it’s unrelated to the filter this
ticket focuses on. This ticket was created specifically to ensure we didn't
lose the work contributed by one of our team members, Kalina, whose efforts
have made the main page look much more appealing.
This PR introduces a new feature but also breaks existing
functionality(Category Tabs overlaps dropdowns in the Header). I’m not sure
it’s a good practice to merge a feature that negatively impacts other
working components.
Would it be better to refine this PR to ensure it doesn’t affect other UI
elements before merging? This way, we can avoid introducing bugs to the
Home page, especially since fixing it before Demo Day might not be
possible. What do you think about it?
—
Reply to this email directly, view it on GitHub
<#317 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A25VU37C32SKLEPRR7ZNTP32DYODRAVCNFSM6AAAAABSOYVGGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJVGU3TIMZXGY>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
|
I created this component outside of the original plan and in my free time, simply as an addition to the overall project. I believe it plays an important and significant role. Unfortunately, I no longer have time to fix minor details. I think it should be quite simple to resolve the z-index issue, possibly somewhere in the header. This would take no more than two minutes. However, as I’ve mentioned, unfortunately I no longer have the time to work on this. |
Natalia, I want to merge this component to the main. I spent half a day struggling with my suggestions until I found a solution, and simply changing z-index in the Header to a higher value will completely break the overlay in your new component. So, we need another solution that works for all components. I mentioned a solution earlier that could fix this issue. If you don't have time right now, with your permission, I could make some changes to resolve this problem. |
|
I agree with @grabinskij - you need to pull this overlay component out of the category components. The reason being - the category component already has a Z-index applied putting it behind the header, and no matter what changes you make to the popup's z-index priority, it will always be limited to whatever its parent's is set at. Pulling the popup component out to the root of the document, using callbacks to open it, and then setting it at a z-index above 500 will do the trick :) |
|
Yeah… thanks. We discussed about it yesterday 🙌🏻
…On Fri 6. Dec 2024 at 12:00, Logan Bentley ***@***.***> wrote:
I agree with @grabinskij <https://github.com/grabinskij> - you need to
pull this overlay component out of the category components. The reason
being - the category component already has a Z-index applied putting it
behind the header, and no matter what changes you make to the popup's
z-index priority, it will always be limited to whatever its parent's is set
at.
Pulling the popup component out to the root of the document, using
callbacks to open it, and then setting it at a z-index above 500 will do
the trick :)
—
Reply to this email directly, view it on GitHub
<#317 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A25VU35PQZQ4ICD6FE3G4O32EF7TNAVCNFSM6AAAAABSOYVGGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRSHA3DEOJXGU>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
Implemented a simplified version of the Range Price Popup modal.
The modal is designed to open upon clicking the FilterButton.
Includes a modular structure to easily add new features or enhancements in the future.
Updated the slider functionality and styles to reflect dynamic price ranges and color changes for better UI/UX alignment.