[50307] Timestamp overflows the BorderBox on Storages index page #13923
Conversation
…orages-index-page
…orages-index-page
…orages-index-page
…ifferent breakpoints
…orages-index-page
There was a problem hiding this comment.
Hi @bsatarnejad
As discussed, please get rid of the doubled avatars by applying a class.
| @@ -1,9 +1,9 @@ | |||
| <%= | |||
| if storages.present? | |||
| render(Primer::Beta::BorderBox.new) do |component| | |||
| render(Primer::Beta::BorderBox.new(classes: 'op-storage-list--box')) do |component| | |||
There was a problem hiding this comment.
What is this new class needed for?
| @media screen and (max-width: $breakpoint-md) | ||
| .op-storage-list--header--name | ||
| min-width: 140px | ||
| .op-storage-list--row--name, op-storage-list--row--host |
There was a problem hiding this comment.
The second selector is missing a dot, otherwise it won't get applied. That being said, it might be that it is necessary ?
| .op-storage-list--header--name | ||
| min-width: 80px | ||
|
|
||
| .op-storage-list--row--name, op-storage-list--row--host |
|
|
||
| @media screen and (max-width: $breakpoint-md) | ||
| .op-storage-list--header--name | ||
| min-width: 140px |
There was a problem hiding this comment.
The min-width can be replaced by using minmax(140px, 1fr) in the grid definition
…orages-index-page
…orages-index-page
…orages-index-page
…ex-page' of github.com:opf/openproject into 50307-timestamp-overflows-the-borderbox-on-storages-index-page
…me of avatar for mini laptop and tablet
…orages-index-page
Hi @HDinger |
There was a problem hiding this comment.
Hi @bsatarnejad
I have left some remarks. I think we can reduce the amount of touched files by adding a default to the new parameter. Thus you can undo all the changes to other places where the renderer is called. This will make the PR a bit more clear.
| @@ -21,6 +21,7 @@ export interface AvatarOptions { | |||
| export interface NameOptions { | |||
| hide:boolean; | |||
| link:boolean; | |||
| classes:string; | |||
There was a problem hiding this comment.
Please make that optional, so that you don't have to pass an empty string everywhere.
| @@ -164,6 +166,7 @@ export class PrincipalRendererService { | |||
| span.textContent = principal.name; | |||
| span.classList.add('op-principal--name'); | |||
| span.title = title; | |||
| classes!== '' && span.classList.add(classes); | |||
There was a problem hiding this comment.
There is a space missing, and I think the check is unnecessary, isn't it?
There was a problem hiding this comment.
Actually it is needed. Without it, it raises an error of accessing null value.
| @@ -32,7 +32,7 @@ class AvatarComponent < ApplicationComponent | |||
| include AvatarHelper | |||
| include OpPrimer::ComponentHelpers | |||
|
|
|||
| def initialize(user:, show_name: true, link: true, size: 'default', classes: '', title: nil) | |||
| def initialize(user:, show_name: true, link: true, size: 'default', classes: '', title: nil, classes_for_name: '') | |||
There was a problem hiding this comment.
I am not super happy with that name. My suggestion would be name_classes, which is not really better tbh 🙈 but at least it is consistent to how we name this object in other classes 🤷♀️
Co-authored-by: Henriette Darge <[email protected]>
…vatar for tablet and small screen laptop
There was a problem hiding this comment.
As we discussed, it now looks weird to have two classes named ..-only. So please change it to hidden-for-tablet-and-small-laptops and change the breakpoints accordingly. I am really sorry for the extra round 🙈 🙈
Other than that, it looks fine 👍 So you can simpley merge the PR once the class name is changed and the CI is green. Please use the "squash" option for the merge, so that we have a cleaner history.
bsatarnejad
deleted the
50307-timestamp-overflows-the-borderbox-on-storages-index-page
branch
https://community.openproject.org/work_packages/50307/activity