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

MM-24490 - Updating UI for link previews name #5401

Merged
merged 1 commit into from
May 7, 2020
Merged

MM-24490 - Updating UI for link previews name #5401

merged 1 commit into from
May 7, 2020

Conversation

asaadmahmood
Copy link
Contributor

Summary

MM-24490 - Updating UI for link previews name

Ticket Link

https://mattermost.atlassian.net/browse/MM-24490

Screenshots

Screenshot 2020-04-27 at 6 47 22 PM

@asaadmahmood asaadmahmood added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Apr 27, 2020
@asaadmahmood asaadmahmood added this to the v5.24.0 milestone Apr 27, 2020
larkox
larkox suggested changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

In general LGTM, just two small things.

components/post_view/post_body_additional_content/post_body_additional_content.jsx Outdated Show resolved Hide resolved
components/single_image_view/single_image_view.jsx Outdated Show resolved Hide resolved
@esethna esethna removed the 1: PM Review Requires review by a product manager label Apr 27, 2020
@lindy65 lindy65 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Apr 27, 2020
@mm-cloud-bot mm-cloud-bot removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Apr 27, 2020
@lindy65 lindy65 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Apr 28, 2020
@mm-cloud-bot mm-cloud-bot removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Apr 28, 2020
@asaadmahmood
Copy link
Contributor Author

Done @jespino @larkox

larkox
larkox approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM

jespino
jespino approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

LGTM

@jespino jespino removed the 2: Dev Review Requires review by a core commiter label Apr 28, 2020
@esethna esethna added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Apr 28, 2020
@esethna
Copy link
Contributor

esethna commented Apr 28, 2020

@cpanato this test server looks like it's having issues starting up? Can you help take a look? Thanks!

@mm-cloud-bot mm-cloud-bot removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Apr 28, 2020
@esethna esethna added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Apr 29, 2020
@esethna
Copy link
Contributor

esethna commented Apr 29, 2020

@asaadmahmood not sure we want to update the link styling? See cases where this breaks:
image

@esethna esethna added 1: PM Review Requires review by a product manager 1: UX Review Requires review by a UX Designer labels Apr 29, 2020
esethna
esethna suggested changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment

Choose a reason for hiding this comment

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

@asaadmahmood not sure we want to update the link styling? See cases where this breaks:
image

@asaadmahmood
Copy link
Contributor Author

Yup @esethna. Looks like there are some other cases that would need a larger refactor, so I think we can skip this for now.

Screenshot 2020-04-29 at 2 08 15 PM

@asaadmahmood asaadmahmood reopened this May 3, 2020
@andrewbrown00
Copy link

@andrewbrown00 We have to enable link previews in the system console first, but I'll do that when I make my change to the PR.

Hmm, I did that but the caret never showed up. Also, should we change the font-size to be consistent with the font size for images?

@asaadmahmood asaadmahmood added Setup Cloud Test Server Setup a test server using Mattermost Cloud and removed Setup Cloud Test Server Setup a test server using Mattermost Cloud labels May 4, 2020
@asaadmahmood
Copy link
Contributor Author

@andrewbrown00 that would be a bit hard to do, as the link for the preview is not easily distinguishable right now.
It can be the first one, or the middle one in a message.

Screenshot 2020-05-04 at 7 27 53 PM

@asaadmahmood
Copy link
Contributor Author

asaadmahmood commented May 4, 2020 •
edited
Loading

I've enabled the previews and posted a link in the PR. @andrewbrown00

@andrewbrown00
Copy link

Thanks @asaadmahmood looks good.

image

What's the dev effort to move the icon to the right to be consistent with images?

@asaadmahmood
Copy link
Contributor Author

@andrewbrown00 Not sure, maybe @hmhealey knows as he worked on the markdown stuff.

@esethna
Copy link
Contributor

esethna commented May 5, 2020

Can we do anything about this case as part of this ticket for fixing the alignmnet issues?
image

@asaadmahmood
Copy link
Contributor Author

@esethna Not much, its quite hard, I can move the icon somewhere at the top, but then there may be cases where the preview is being generated by an image that's not at the start but somewhere at the middle, so I would suggest we look at that when we can look at this issue in detail.

@andrewbrown00 andrewbrown00 self-requested a review May 5, 2020 18:28
@andrewbrown00 andrewbrown00 removed the 1: UX Review Requires review by a UX Designer label May 5, 2020
@esethna esethna removed the 1: PM Review Requires review by a product manager label May 6, 2020
@lindy65
Copy link
Contributor

lindy65 commented May 7, 2020

Hey @asaadmahmood,

Is the icon supposed to be left-aligned with the start of the username or positioned half-way between the first and second letter of the username as per the screenshot below?

image

Compared to the screenshot ES posted to the ticket as an example of what should be fixed, it seems the icon has been moved only marginally left? Is this correct?

Screenshot from ticket:
image

@asaadmahmood
Copy link
Contributor Author

@lindy65 Expected, as its a button, so it's not aligned.

@asaadmahmood
Copy link
Contributor Author

And based on the ticket, the main fix is the icon's vertical alignment with the text, it has moved up a bit.

lindy65
lindy65 approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@lindy65 lindy65 left a comment

Choose a reason for hiding this comment

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

Thanks @asaadmahmood - got it :) Looks good to me then 👍

@lindy65 lindy65 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels May 7, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@asaadmahmood asaadmahmood merged commit 58a1e62 into mattermost:master May 7, 2020
@asaadmahmood asaadmahmood deleted the MM-24490 branch May 7, 2020 17:44
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 8, 2020
bradjcoughlin pushed a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 19, 2020
@asaadmahmood @bradjcoughlin
Updating spacing (mattermost#5401)
f34fe39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jespino jespino jespino approved these changes

@larkox larkox larkox approved these changes

@lindy65 lindy65 lindy65 approved these changes

@esethna esethna esethna approved these changes

@andrewbrown00 andrewbrown00 andrewbrown00 approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation
Projects
None yet
Milestone
v5.24.0
Development

Successfully merging this pull request may close these issues.
8 participants
@asaadmahmood @esethna @andrewbrown00 @lindy65 @mm-cloud-bot @jespino @larkox @amyblais