-
-
Notifications
You must be signed in to change notification settings - Fork 833
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.
I didn't have the time to go through everything but generally, this looks awesome! Just some minor things and questions. Thank you! 🚀
The stuff with the prefixes is probably up to the team.
And feel free to scream, if my suggestions don't make sense 😅
Edit: Also, regarding the curly braces... we've settled on { foo, bar }
, instead of {foo, bar}
for objects but a rule for the react-sdk
hasn't been setup yet
Co-authored-by: Šimon Brandner <[email protected]>
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.
This would probably be for the second part of the review. I haven't made myself review the EventTile.tsx
and _BubbleLayout.scss
changes yet, but I think we should probably first decide on the things like the prefixes
src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx
Outdated
Show resolved
Hide resolved
src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx
Outdated
Show resolved
Hide resolved
src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx
Outdated
Show resolved
Hide resolved
src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx
Outdated
Show resolved
Hide resolved
src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx
Outdated
Show resolved
Hide resolved
src/components/views/settings/tabs/user/AppearanceUserSettingsTab.tsx
Outdated
Show resolved
Hide resolved
singleSideBubbles: false, | ||
adaptiveSideBubbles: false, |
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.
I am probably totally overlooking where this is used
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.
Once a build failed if singleSideBubbles
wasn't there. Recently I noticed that adaptiveSideBubbles
was missing and added it just in case. But I have absolutely no clue why this is needed either.
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.
Odd
Revert "Add some strings that sprinkled away" This reverts commit 4a12d62.
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 otherwise. I have to admit that I've mostly skimmed through the CSS and the EventTile
code. I am starting to understand why reviews can take a while sometimes 😅
Co-authored-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
hey @su-ex can we get some screenshots in the PR description to make review easier? Thanks! |
@SimonBrandner now that your name is on the commits as well, we'll need signoff from you on here as well please. |
Ah, true:
|
@turt2live Sure, added! 📷 |
@su-ex thanks for cherry picking your commits & iterating on your implementation into this PR after the discussion we had on Matrix, really appreciated! The PR principally looks good, but we need to do a bit of internal planning (with @gsouquet @amshakal & @jryans) on precisely how we'll iterate on this so will figure out some details with them before we approve/merge. Relatedly, we wouldn't consider element-hq/element-web#4635 closed until we're closer to shipping this on production, including de-risking some cross-platform efforts, so I've edited your PR description to avoid this. Thanks again for the effort! |
Related to element-hq/element-web#4635
Screenshots
Signed-off-by: Quirin Götz [email protected]