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

Rollout expanded set of Link Unfurl Cards for desktop #10585

Closed
benjthayer opened this issue May 8, 2023 · 15 comments · Fixed by #12145
Closed

Rollout expanded set of Link Unfurl Cards for desktop #10585

benjthayer opened this issue May 8, 2023 · 15 comments · Fixed by #12145
Assignees
Labels
Design-QA E:Desktop New Unfurling API Implementation of the new unfurling API for all links
Milestone

Comments

@benjthayer
Copy link

benjthayer commented May 8, 2023

Dependent on #10852

New designs have been created to complete the set of unfurl cards for desktop.

The designs now include the unfurl cards for:

  • YouTube
  • GitHub
  • Twitter
  • External links
  • Internal Communities
  • Internal Community Channels
  • Internal Profiles
  • GIFs

https://www.figma.com/file/Mr3rqxxgKJ2zMQ06UAKiWL/%F0%9F%92%AC-Chat%E2%8E%9CDesktop?type=design&node-id=21932-655154&t=i3leEXxXB1cWXfuL-4

Also worth noting that there a loading/fetch state for the Community banner for the internal Community and channel cards:

Screenshot 2023-05-08 at 18 16 53

The designs aim to be as close to the mobile implementation @ilmotta is currently working on so whoever picks this task up, please do set up a call with @ilmotta (and myself if helpful) to discuss what work has already been undertaken on the dev side to limit duplication.

cc @John-44

@John-44 John-44 added this to the 0.13 milestone May 8, 2023
@ilmotta
Copy link

ilmotta commented May 8, 2023

Thanks @benjthayer! For the moment, I would say the best for the desktop team is to wait until the status-go implementation stabilizes this month, since it's still highly susceptible to changes, even at the protocol layer (I'll open the first PR this week in status-go).

I'll make sure to make another comment in this issue once I think the status-go implementation is solid enough to be used.

@benjthayer
Copy link
Author

Thanks @benjthayer! For the moment, I would say the best for the desktop team is to wait until the status-go implementation stabilizes this month, since it's still highly susceptible to changes, even at the protocol layer (I'll open the first PR this week in status-go).

I'll make sure to make another comment in this issue once I think the status-go implementation is solid enough to be used.

Thanks @ilmotta !

@caybro caybro added the ui-team label May 9, 2023
@caybro
Copy link
Member

caybro commented May 9, 2023

@benjthayer ping, how much do you think the Figma design will change? We should not imho wait that long and should start working on the UI components right now (even if it's going to be fed with dummy data for now, until the status-go backend stabilizes)

@John-44
Copy link

John-44 commented May 9, 2023

@benjthayer ping, how much do you think the Figma design will change? We should not imho wait that long and should start working on the UI components right now (even if it's going to be fed with dummy data for now, until the status-go backend stabilizes)

@caybro I don't think these designs will change, and I agree it would be a good idea to start working on the UI components with dummy data now. Then when @ilmotta let's folks know here that the status-go implementation has stabilised, the UI components can then be wired to the backed at that point.

@caybro anything that has the potential to make a big improvement to Status Desktop's performance has priority over this task of course

@ilmotta
Copy link

ilmotta commented May 9, 2023

Hi @caybro. At this very moment I'm working on the mobile side and just yesterday me and a couple other designers, including Ben and John, discussed a bunch of details, but the designs are pretty stable as far as I understand.

I believe you'll be able to start working on the desktop side without dummy data as early as next week, since I'm going to open the status-go PR this week (it may be a bit rough on the edges, but totally usable). But as you said, I started with dummy data on the client in the beginning too, so that works of course.

Let's keep in touch since the new link unfurling code will still evolve a couple PRs down the road. Thanks

Then when @ilmotta let's folks know here that the status-go implementation has stabilised, the UI components can then be wired to the backed at that point.

Will do 👍 @John-44

@caybro caybro self-assigned this May 9, 2023
@caybro
Copy link
Member

caybro commented May 9, 2023

@benjthayer ping, how much do you think the Figma design will change? We should not imho wait that long and should start working on the UI components right now (even if it's going to be fed with dummy data for now, until the status-go backend stabilizes)

@caybro I don't think these designs will change, and I agree it would be a good idea to start working on the UI components with dummy data now. Then when @ilmotta let's folks know here that the status-go implementation has stabilised, the UI components can then be wired to the backed at that point.

@caybro anything that has the potential to make a big improvement to Status Desktop's performance has priority over this task of course

Got it, that was my plan; finish the SHA1 work and then eventually start the UI component work with dummy models/data until the backend has stabilized

@ilmotta
Copy link

ilmotta commented May 16, 2023

@benjthayer, @John-44, @pedro-et: FYI Link previews are being tested by QAs already. If you want, you can test right now the PR builds status-im/status-mobile#15891 (comment).

I was going to initially implement the whole feature behind a feature flag, but since we're aiming to include it in the next mobile release this week, it'll be open for all of us.

There's much to improve behind the scenes (status-go), but it should be enough for the desktop team to start the work if they can/desire (@caybro).

Cheers!

@igor-sirotin
Copy link
Contributor

Want to add some new requirements:

  1. After Unfurl image URLs status-go#3761 we should be able to show an image preview in the same way.
    We should also add some UI like Open original URL in Browser to be able to see the original size image.
  2. After Unfurl Status deep links status-go#3762 we should be able to show the link preview in the same way.
    We should also fetch the latest link data from waku when the link previews appears in the viewport. We could do this automatically in status-go on preview reception, but it might lead to ddos attacks 🙂
  3. Opening any preview should open a popup asking Do you trust <hostname>?: cancel/open. And a checkbox `never ask about

@caybro caybro added the E:Desktop New Unfurling API Implementation of the new unfurling API for all links label Jul 19, 2023
@caybro caybro assigned alexjba and unassigned caybro Aug 30, 2023
@alexjba
Copy link
Contributor

alexjba commented Sep 11, 2023

@igor-sirotin Do you know if there's a backend task to provide the favicon of the unfurled URL? I see it's currently missing

@ilmotta
Copy link

ilmotta commented Sep 12, 2023

@alexjba, back then, I created issues in status-mobile because only mobile was using the new link preview API. Now that the desktop app is also implementing link previews per the new API, I agree with you it would be nice to have the issues in status-go too. In retrospect, I regret not creating them in status-go.

Here's the issue about favicons in status-mobile: status-im/status-mobile#15910

And all the issues created in status-mobile related to link previews https://github.com/status-im/status-mobile/issues?q=is%3Aopen+is%3Aissue+label%3A%22E%3AMobile+URL+Preview+MVP%22

@alexjba
Copy link
Contributor

alexjba commented Sep 13, 2023

@ilmotta Thank you for the info. Helps a lot!

One thing I see missing from the new APIs is the domain name (we have the hostName which includes the prefix) needed for the image previews. Is it something that will be implemented in status-go as part of this task status-im/status-mobile#17073? I see both mobile and desktop designs are using a logo for the image preview containing the first letter of the domain name.

Screenshot 2023-09-13 at 11 36 58 Screenshot 2023-09-13 at 11 50 38

Desktop: https://www.figma.com/file/Mr3rqxxgKJ2zMQ06UAKiWL/💬-Chat⎜Desktop?type=design&node-id=22347-219483&mode=design&t=Pzn7RybClrTp2eUF-0
Mobile: https://www.figma.com/file/wA8Epdki2OWa8Vr067PCNQ/Composer-for-Mobile?type=design&node-id=9598-551920&mode=design&t=ruX4jUgKgJQaFxd8-0

@alexjba alexjba linked a pull request Sep 13, 2023 that will close this issue
1 task
@ilmotta
Copy link

ilmotta commented Sep 13, 2023

One thing I see missing from the new APIs is the domain name (we have the hostName which includes the prefix) needed for the image previews. Is it something that will be implemented in status-go as part of this task status-im/status-mobile#17073? I see both mobile and desktop designs are using a logo for the image preview containing the first letter of the domain name.

Ah, I think I understand your question now @alexjba. Figma designs recently changed because you can see in the screenshot I took in status-im/status-mobile#17073 that the small top left circle had the image preview itself and there was no first letter of the domain to show.

About the S prefix shown in the following image, I don't understand the value for the user, at least in the Figma example. @benjthayer, could you help us understand this requirement? We are already showing the hostname i.seadn.io, so what does S convey?

https://www.figma.com/file/wA8Epdki2OWa8Vr067PCNQ/Composer-for-Mobile?type=design&node-id=9598-47319&mode=design&t=yb6qaIe9hClI22Xm-0

@benjthayer
Copy link
Author

About the S prefix shown in the following image, I don't understand the value for the user, at least in the Figma example. @benjthayer, could you help us understand this requirement? We are already showing the hostname i.seadn.io, so what does S convey?

Hey @ilmotta the S is a dynamic representation of the URL if there is no favicon - the S in this case is an abbreviation of seadn - much like how we use letters for user's pfp when they have not uploaded an image. While it can be considered duplicative of the URL, it is in place as much to standardise the design for all cards - so there is the same layout and margins etc between previews with and without favicons.

cc @mariocnovoa

@ilmotta
Copy link

ilmotta commented Sep 13, 2023

Thanks @benjthayer, yeah consistency is important too. I still find S redundant because the hostname is there already and it's a reliable information to the user, whereas S is an abbreviation that can happen for many different hostnames.

@alexjba
Copy link
Contributor

alexjba commented Sep 18, 2023

Wrongfully closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design-QA E:Desktop New Unfurling API Implementation of the new unfurling API for all links
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants