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

Fix hidden invite link area except transfer dialog and devtools button area #8165

Closed
wants to merge 12 commits into from
Closed

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 26, 2022

Fixes element-hq/element-web#20911
Closes #7966

  • Disable overflow: hidden on the invite dialog with footer, except the transfer dialog for now
  • Remove the height setting with a magic number
  • Style .mx_CopyableText area with white-space: nowrap to make text ellipsis work
  • Reset the default button margin to put the button at the bottom of the dialog

As long as the height is specified with a magic number, the area is going to be hidden anyway.

The overflow style was introduced with here (based on here) related to dialpad improvements, so this PR avoids applying the style to the transfer dialog with :not(.mx_InviteDialog_transfer). This exception should be removed after it is confirmed that this change can also be applied to that dialog without a visual regression.

Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com

type: defect


Here's what your changelog entry will look like:

🐛 Bug Fixes

Preview: https://pr8165--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.

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul luixxiul requested a review from a team as a code owner March 26, 2022 12:34
@github-actions github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Mar 26, 2022
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Mar 26, 2022

Codecov Report

Merging #8165 (09b8a6f) into develop (cdb0c2c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #8165   +/-   ##
========================================
  Coverage    29.25%   29.25%           
========================================
  Files          863      863           
  Lines        49876    49876           
  Branches     12696    12696           
========================================
  Hits         14592    14592           
  Misses       35284    35284           

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.

I don't believe this issue is unique to the invite dialog. Having looked at other dialogs, like the Share dialog, devtools, etc, they are all cut off on the bottom by 1 pixel border's worth.

This implies that something else is causing the issue.

@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 4, 2022

The issue on Devtools is actually caused by another height setting on this line of _DevtoolsDialog.scss. I guess any dialogs styled with height: calc(100% - x) should have the same issue, potentially or not.

luixxiul added 5 commits April 6, 2022 15:12
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 7, 2022

I've fixed both issues. Please check again, thanks.

after
after2
after3

It seems to me the fundamental issue here would be that there is not an unified, common way of placing buttons on the bottom of a dialog; it looks like each dialog handles them in its unique way, which should be the root cause of this kind of inconsistencies. However, since normalization is out of scope of this PR, I would leave the task for another one.

@luixxiul luixxiul requested a review from turt2live April 7, 2022 22:06
@luixxiul luixxiul changed the title Fix hidden invite link area except transfer dialog Fix hidden invite link area except transfer dialog and devtools button area Apr 7, 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.

I'm still generally concerned that the change is wider spread than the two dialogs. I'm very mindful that we have half a dozen ways to construct a dialog, but finding the commit/change which caused the regression in the first place would be ideal to help identify all of the affected dialogs.

I don't think we need to introduce flexboxes in the dialogs to fix this.

@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 9, 2022

finding the commit/change which caused the regression in the first place would be ideal to help identify all of the affected dialogs.

Should this PR be closed in favor of another PR which is supposed to normalize the dialogs? I am asking this because this PR first intended to cover the invite dialog (and devtools dialog) only

@turt2live
Copy link
Member

Preferably we expand the scope to fix the root issue rather than a specific dialog. If that means closure, so be it.

@luixxiul luixxiul closed this Apr 9, 2022
@luixxiul luixxiul deleted the InviteDialog2 branch April 9, 2022 05:28
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 9, 2022

I am not really sure whether there would be one ultimate root cause, as each area has been created differently at different timing.

For those who are willing to work on this issue: you should check each dialog with .mx_Dialog to detect which commits introduced what is styled with fixed height (such as the header), and globally replace the style with something else which does not hide the bottom area.

@luixxiul luixxiul restored the InviteDialog2 branch April 9, 2022 14:19
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 9, 2022

I don't think we need to introduce flexboxes in the dialogs to fix this.

I researched a little bit, and I guess that by introducing flexbox it should be able to achieve something similar to https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Flexible_Box_Layout/Typical_Use_Cases_of_Flexbox#card_layout_pushing_footer_down

.card {
    display: flex;
    flex-direction: column;
}

.card .content {
    flex: 1 1 auto;
}

If it is possible to consider the dialog as one card layout, why not applying the same way?

@luixxiul luixxiul reopened this Apr 9, 2022
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 9, 2022

To design team: I would like to know if it is reasonable to apply the card layout for the dialogs to push down the button area to the bottom. Thank you in advance.

@luixxiul luixxiul marked this pull request as draft April 20, 2022 18:11
@MadLittleMods MadLittleMods added the Z-Community-PR Issue is solved by a community member's PR label Jun 1, 2022
@luixxiul luixxiul deleted the InviteDialog2 branch June 2, 2022 14:07
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
3 participants