-
-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Update card summary design #255
Update card summary design #255
Conversation
For max-width of 300px, should we use percentage (%) for better responsive result? Say 80%? @lcd2yyz please see the proposed result where Andrew is trying to keep the datetime in one line. |
@lcd2yyz Just want to add a note based on a discussion we had. The 300px limit will only affect super wide screens. Suppose someone has a very wide screen so that the filter cards are 1000px wide. Then at most the selected value text will cover 1/3 of the card width. On a small screen, the text will be truncated, with '...', if it is too big to fit within the width of the card. |
Upon first look it's great. A few notes though:
I will have a closer look later tonight. |
Thanks for reminding that @chlebowa
|
Agree with this.
Other than the margin, ellipsis looks pretty nice to me. However, I would prefer to add some conditional handling if possible
|
I thought we are moving away from checking the number of level => only using character width of the selected levels? To summarize again to ensure we are aligned
|
Should we keep things that are not part of the summary in separate issue? I could see an exception for NA font size for example; that semi-relates because it's font size compared to summary. |
Yes, we were moving away from checking just the count of levels. When I managed to implement the auto-truncating of text via CSS, I posed the question of whether this was sufficient to replace counting number of characters. @chlebowa has a good point about levels with long names. Personally I think '...' at the end of the text effectively communicates that there is more to the story so for details expand the card. I think there is some confusion between the truncating of text with CSS and what content is populated in the section the CSS applies to. I was thinking the current CSS is sufficient to where we don't have to worry about counting levels or characters. We just put the selected values in and let CSS adjust the display. If it is not sufficient, that's perfectly fine. As far as the 300px, that has nothing to do with what text we put in the card. In fact, there isn't a way to check how many pixels a given selection of text will be (a reliable way at least). The only thing we can do is count characters and try to pick a value that will work. Again, I thought the CSS would be sufficient to where we don't need to do this but if not that's fine. |
…r_panel_refactor@main
New issue to handle the css part of filter card: |
I am leaning toward this as well:
Other than this, this PR looks good. |
I thought the purpose of the ellipsis is to handle the potential overflow due to screen size difference. The presence of ellipsis would signal that there is more than what user sees, which (hopefully) prompt the user to check by expanding the card. The reason why I suggested to use a cutoff limit based on character length, is because of potential categories with very short character lengths, like SEX (F, M, U) or ARMCD (A, B, C), where most likely all can fit in one line (same reason why treating logical and binary vars differently). The summary will be much more informative to the app users to see all selected values, than just "N level selected". For now, I'm okay with using rule |
@lcd2yyz @donyunardi Right now this code displays categorical values depending on length of the text for the selected values. So it does attempt to display e.g. "M, F, U". If however the text is too long (40 character magic number right now), then "n levels selected" is used. |
@donyunardi and I reviewed the UI together this afternoon, I really like it! Exactly what I had in mind! |
There is a separate issue for that: #236 |
Updates the summary row of the card header.
Fixes #220