Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fixes the padding to make the border of invite link visible #7966

Closed

Conversation

archijaiswal
Copy link

@archijaiswal archijaiswal commented Mar 3, 2022

Fixes element-hq/element-web/issues/20911

Modified the padding on dialog button to make the border of invite link visible.

How it looked before

k1

How it looks now

k2

Signed-off-by: Archi Jaiswal [email protected]


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fixes the padding to make the border of invite link visible (#7966). Contributed by @archijaiswal.

Preview: https://pr7966--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

fixed padding on dialog button to make border of invite link visible
@archijaiswal archijaiswal requested a review from a team as a code owner March 3, 2022 09:41
@dbkr dbkr requested a review from janogarcia March 3, 2022 14:24
@SimonBrandner
Copy link
Contributor

@archijaiswal, could you please rename the PR to something more understandable (e.g. Fix the padding in invite dialog...) and merge and push the latest change in the js-sdk to your fork? It should fix the type checks

Thanks

@archijaiswal archijaiswal changed the title Fixes vector-im/element-web/issues/20911 Fixes the padding to make the border of invite link visible Mar 3, 2022
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for taking a look at this! As mentioned in the thread, I'm somewhat concerned that the issue is a bit more involved than just fixing padding - if you have more information about how the dialog is behaving to cause the issue, that would be appreciated.

@@ -22,7 +22,7 @@ limitations under the License.
border: solid 1px $light-fg-color;
margin-bottom: 10px;
margin-top: 10px;
padding: 10px;
padding: 9px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9px is a bit of a weird value in our code (we typically stick to multiples of 2 or 4, or at least even numbers) - can this get a comment for why it's a bit different?

More generally though, I'm concerned that this doesn't fix the issue for the future. The dialog should be able to handle additional padding, implying something is going wrong in the dialog's CSS rather than here, I think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, alright. Thank you so much for your kind review. I'll surely keep these things in mind and try to come up with a better fix.
(Nothing so specific for 9px since it was the minimum modification I could think of to fix this. Thanks for your direction!)

@archijaiswal
Copy link
Author

Please review this @turt2live , 1px was causing this issue.

Thanks

@turt2live turt2live self-requested a review March 4, 2022 15:52
@luixxiul
Copy link
Contributor

luixxiul commented Mar 5, 2022

I am wondering why removing overflow: hidden from .mx_InviteDialog_content would not work instead.

@archijaiswal
Copy link
Author

Actually, that was unnecessarily adding a scroll bar as it's height is fixed. @luixxiul

@luixxiul
Copy link
Contributor

luixxiul commented Mar 5, 2022

Actually, that was unnecessarily adding a scroll bar as it's height is fixed. @luixxiul

This is what I see by removing the overflow property. Maybe on my side only?

Peek 2022-03-05 10-37

The overflow has been added here (based on here) related to dialpad improvements, so not sure if it could be safely removed.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh, I forgot that there was a PR already open for this.

Like in my other review, I think this is a more general problem than the invite dialog: #8165 (review)

@MadLittleMods MadLittleMods added Z-Community-PR Issue is solved by a community member's PR T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems labels Jun 1, 2022
@turt2live
Copy link
Member

It appears as though the bug which caused this has been fixed - thanks for the PR in any case :)

@turt2live turt2live closed this Oct 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some dialogs are cut off on the bottom edge
5 participants