Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

left-sidebar: Allow upto 2 lines for PMs #21021

Merged
merged 2 commits into from Feb 4, 2022
Merged

left-sidebar: Allow upto 2 lines for PMs #21021

merged 2 commits into from Feb 4, 2022

Conversation

jai2201
Copy link
Collaborator

@jai2201 jai2201 commented Feb 2, 2022

#21003

GIFs or screenshots:
Screenshot from 2022-02-02 15-03-31

@jai2201
Copy link
Collaborator Author

jai2201 commented Feb 2, 2022

@timabbott
I had to use
display: -webkit-box
for performing this action with the use of only CSS, as normally text-overflow: ellipsis; works only for 1 line, but if we want to use it for 2 lines, we would need display as -webkit-box

and style-lint won't allow me using this property as display, so had to disable stylelint on this specific CSS property.

It would be great if there's any other alternative to allow this display property through style-lint.

overflow: hidden;
text-overflow: ellipsis;
padding-right: 2px;
-webkit-line-clamp: 2;
-webkit-box-orient: vertical;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

What does this do on non-webkit browsers? Also, what linter rule are you disabling?

Copy link
Collaborator Author

@jai2201 jai2201 Feb 2, 2022

Choose a reason for hiding this comment

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

@timabbott I disabled the all stylelint rules for these 5 lines of CSS as display: -webit-box is not a valid for display property in stylelint library.

and on non-webkit browers, these CSS ofc won't work if we want to go with CSS only method.
But now a days, Every browser supports -webkit except Internet Explorer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's also an other alternative method to do this change, but it would involve JS -

Using the Clamp.JS module.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can you post the caniuse details confirming that this will work in all browsers, and test in firefox? It's important for reviewers to independently verify those details.

Copy link
Collaborator Author

@jai2201 jai2201 Feb 4, 2022

Choose a reason for hiding this comment

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

Hey Tim , I tested this on firefox and it's working perfectly fine!
also here's the ss of caniuse

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot from 2022-02-04 17-49-07
Screenshot from 2022-02-04 10-15-08

@jai2201 jai2201 requested a review from andersk February 3, 2022 08:38
@jai2201
Copy link
Collaborator Author

jai2201 commented Feb 3, 2022

@andersk, I have made the changes you asked for, please have a look.

@amanagr
Copy link
Member

amanagr commented Feb 4, 2022

Another option is to show pre-formatted text using white-space: pre, but the method in this pr is better if it works. We do have to write something like to pre-format the text:

    let names = get_display_full_names(other_ids).sort().join(", ");
    if (names.length > 26) {
        names = names.slice(0, 27) + "\n" + names.slice(27);
    }

    if (names.length > 55) {
        names = names.slice(0, 55) + "...";
    }

@jai2201
Copy link
Collaborator Author

jai2201 commented Feb 4, 2022

Another option is to show pre-formatted text using white-space: pre, but the method in this pr is better if it works. We do have to write something like to pre-format the text:

    let names = get_display_full_names(other_ids).sort().join(", ");
    if (names.length > 26) {
        names = names.slice(0, 27) + "\n" + names.slice(27);
    }

    if (names.length > 55) {
        names = names.slice(0, 55) + "...";
    }

yeah, this method ofc is available, but i wanted to avoid the use of JS for this, which is why i used -webkit.
and it do works in all the modern browsers according to caniuse.

@timabbott timabbott merged commit 97ffe2b into zulip:main Feb 4, 2022
@timabbott
Copy link
Sponsor Member

Squashed and merged, after writing a proper commit message. Thanks @jai2201!

Please read the commit message I used and adjust your future PRs to use that style -- commit messages should explain why a change is being made and why anything potentially unusual in the commits was done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants