-
Notifications
You must be signed in to change notification settings - Fork 48
Make blueprints previews adjustable to window height #1750
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
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.
In terms of this PR, it works as described and the height of the previews increases based on the window height 👍
I am not sure if it was discussed already but I am finding the presence of the scrollbar on the smallest screen a slightly not cohesive in the UI. I am wondering if there was a way to fit everything on the smallest screen so that there is no need for it 🤔
@katinthehatsite Hm, maybe I misunderstood your message, but yes - avoiding scroll om small (default 600px height) screen is the main goal of the trick with previews' height:
|
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.
Nice work, it's looking good!
You might consider using CSS media queries instead of JavaScript for this:
className={cx(
'w-full h-32 object-cover object-top cursor-pointer',
'[@media(min-height:680px)]:h-48', // CSS handles the responsiveness
// ... other classes
)}
Benefits:
- No resize event listeners or state management needed
- Better performance (browser handles natively)
- Simpler code maintenance
What do you think?
@bcotrim thanks, I didn't knwo about this oportunity and Cursor didn't tell me too 😅 |
@katinthehatsite got it, you meant not 900x600, but the smallest possible option. To fix it I will go with what Antonio proposed, to keep tags always inside 1 line. |
@katinthehatsite fixed this way. I think it's enough and it's unnecessary to polish cutting at the right side, otherwise we will add tricky code (for ellipsis or something similar) to cover non-critical edge cases. WDYT? Are you ok with it? |
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.
@nightnei we recently introduced some changes to display a "+X more" element when we have more than 3 categories.
Could we maybe adjust this logic to display that element when the categories don't fit in one line?
It would prevent the categories from being cut

Do you have a proposition of logic to cover it? Any polished solution, most likely, will add a lot of lines of code and I am not sure that it's reasonable... Actually, I find the initial scrollbar approach as the most pragmatic decision. If a user wants to work with the smallest possible screen, then they should expect to have scrollbars etc. WDYT about reverting one-line-approach and keeping scroll for the smallest screen? |
I think for a full solution we would need to observe if the last tag appears fully or not. |
It's an option, but I don't find it as a good approach, since we sacrifice rendering tag for our users due to low amount of users who use the smallest size of window and just to avoid rendering scrollbar.
What about resizing the window, probably it should be recalculated. Also, what if we have 1 very long tag for some blueprint in the future. Personally, for me, it sounds like overkill for a non-critical thing. I would vote for reverting to keepign a few lines of tags for teh smallest screen and seeing scrollbar. But I don't mind any decision. |
The good part is that these tags come from our backend, so changing them should be easy.
That's why I suggest showing two tags in the smallest width, 3 tags in the regular or higher width. Although It might bring issues when the tags are in different languages. |
* Add dynamic category overflow with smart +X more handling * fix magic numbers * add translation
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.
LGTM 👍
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.
Thanks for creating useOverflowItems
✨
Related issues
Proposed Changes
With this PR we increase height of Blueptints' previews when window height is more then 680
Testing Instructions