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

Consider rewriting the Lightbox and embed inside WebView #3426

Open
borisyankov opened this issue Mar 24, 2019 · 4 comments
Open

Consider rewriting the Lightbox and embed inside WebView #3426

borisyankov opened this issue Mar 24, 2019 · 4 comments

Comments

@borisyankov
Copy link
Contributor

Rewriting the Lightbox and embed inside WebView will keep the view inside the HTML browser implementation.

The benefits will be:

  • authentication issues - will be resolved as we already successfully load the image inside the message list
  • caching - we already have the image, no need to reload
  • progressive loading - the default web behavior is better than ours (although caching might even eliminate this)
@AB261
Copy link
Collaborator

AB261 commented Mar 29, 2019

May I work on this @borisyankov ? Anything specific that I need to know?

@borisyankov
Copy link
Contributor Author

That might not be ideal for new contributors.

Also, we need to discuss the viability and how good of an idea that is with @gnprice

@AB261
Copy link
Collaborator

AB261 commented Apr 2, 2019

Alright, thanks for letting me know!

@gnprice
Copy link
Member

gnprice commented Apr 30, 2019

Hmm, interesting idea! I agree this could be appealing.

Note that AFAICT there are no remaining issues with authentication. See #3124 -- it looks like the remaining issue is with properly following a redirect. But that's also an area where a WebView should save us from the sloppy bugs of random components from NPM.

(BTW, this is a perfect illustration of why it's super helpful to explicitly link to previous discussions, rather than just allude to them. We can't evaluate "authentication issues" as a possible benefit without looking at what we've previously discussed about specific issues.)

Another issue this might help us with is #2781.

Would you track down the previous GitHub issues, or Zulip chat threads, where we've discussed caching and progressive loading, and link them here? I'm pretty sure we have.

On the other side of the ledger:

  • This blocks on Improve WebView build system #2690 (or the task that PR represents) -- I wouldn't want to add significant new complexity inside the WebView before getting a decent handle on managing the complexity we have.
  • One potentially significant cost of this, in the complexity of the code, is that all the interaction the lightbox currently has with the rest of the app over normal React and Redux interfaces would need to be lifted onto the postMessage channel. I feel like that's a pretty significant source of complexity in the area where we do that today, for the message list. (Which we tolerate because the WebView gives us so much value there.)

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

No branches or pull requests

3 participants