-
Notifications
You must be signed in to change notification settings - Fork 32
Update session details styles to closer match latest Figma #2439
Conversation
Deploying with Cloudflare Pages
|
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.
The generic session details component still feels messy, but I'm fine with refactoring that later.
Left a few comments but other than that LGTM
frontend/src/components/SessionDetail/SessionDetails.module.css
Outdated
Show resolved
Hide resolved
frontend/src/components/SessionDetail/SessionDetails.module.css
Outdated
Show resolved
Hide resolved
frontend/src/components/SessionDetail/SessionDetails.module.css
Outdated
Show resolved
Hide resolved
Co-authored-by: Quentin Gliech <[email protected]>
That component gets deleted by the end of the PR, no? Or do you mean the |
I meant that this component has a lot of logic in it. Sure, it avoids repeating stuff across the three sessions, but I prefer the approach of composing different components like on the session card: matrix-authentication-service/frontend/src/components/BrowserSession.tsx Lines 119 to 163 in 6eb6209
|
I think this is key to a consistent coherent design, otherwise you have to manually sync the 3 session components for copy and order to keep them consistent with each other. Personally not a fan, but at the end of the day this is your project |
I'm fine with leaving it like that for now 👍 |
For #2147