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

Fixing readme loading: Using webview #718

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MattiaPrimavera
Copy link

@MattiaPrimavera MattiaPrimavera commented Sep 11, 2017

I tried to use a WebView for solving #661

It seems like it does work, no scrolling issues, since vertical one is disabled (still provided by the parent view) while the horizontal scroll stays enabled on the WebView.

This leads to :

  • deleting FillReadmeTask
  • deleting lifecycle methods linked to HttpImageGetter
  • delegation of html content rendering and images loading to the WebView

@maniac103
Copy link
Collaborator

#661 already is fixed, though.

@MattiaPrimavera
Copy link
Author

What I meant is that in one of last discussion I've been told that WebView has been avoided to load the Readme and a custom solution had been prefer because of scrolling issues of nested views ...

Wouldn't it be nice to delegate all this custom implemented behaviour to WebView ?

@maniac103
Copy link
Collaborator

What I meant is that in one of last discussion I've been told that WebView has been avoided to load the Readme and a custom solution had been prefer because of scrolling issues of nested views ...

Yes, among other things, in particular styling. For comparison, two screenshots of the Octodroid repo being displayed, with current solution and WebView:

Current:
screenshot_1505113971

WebView:
screenshot_1505114136

The WebView styling simply doesn't fit in as well, at least not without applying a lot of CSS magic.
Also, please note that we can't use horizontal scrolling, because horizontal swipes switch between tabs.

Wouldn't it be nice to delegate all this custom implemented behaviour to WebView ?

Maybe, but we'll need to keep the custom handling for issue/PR comments anyway.

@MattiaPrimavera
Copy link
Author

Maybe, but we'll need to keep the custom handling for issue/PR comments anyway.

Why is that? I obviously considered possibility of adjusting the solution, for the moment it was just trying to check if it was possible.

Shouldn't the WebView handle horizontal scrolling for itself while swipe to move to other tabs should be provided by the activity?

@maniac103
Copy link
Collaborator

Why is that? I obviously considered possibility of adjusting the solution, for the moment it was just trying to check if it was possible.

Because having one WebView per issue comment view totally destroys performance. Yes, we've tried.

Shouldn't the WebView handle horizontal scrolling for itself while swipe to move to other tabs should be provided by the activity?

Yes, but different horizontal swipe behaviour depending on where you swipe is bad from a UX perspective - it's just confusing.

@MattiaPrimavera
Copy link
Author

Because having one WebView per issue comment view totally destroys performance. Yes, we've tried

Yep I can imagine ...

Yes, but different horizontal swipe behaviour depending on where you swipe is bad from a UX perspective - it's just confusing.

Isn't this what the app currently provides in RepositoryFragment?

@maniac103
Copy link
Collaborator

maniac103 commented Sep 11, 2017

Isn't this what the app currently provides in RepositoryFragment?

No, RepositoryFragment doesn't scroll horizontally.

@Tunous
Copy link
Collaborator

Tunous commented Sep 11, 2017

Yes, but different horizontal swipe behaviour depending on where you swipe is bad from a UX perspective - it's just confusing.

Generally yes, but if the target area is easily distinguishable (like code blocks) then there should be no confusion.

@MattiaPrimavera Did you test how horizontal scrolling works in code blocks with long text. From my experience WebView doesn't work well with ViewPager. (But maybe they fixed it)

Right now this pull request is a no go. Not before the WebView gets styling rules to look similar to what TextView based readme looks like. And I think we actually need these styling rules for the preview in comment editor so pull request improving that would be great on its own.

EDIT: In my opinion WebView based readme would be benefit just because of these scrollable code blocks. I really dislike how they are wrapped in TextView and lots of Readme files contain examples in code blocks.

@MattiaPrimavera
Copy link
Author

@Tunous well I tried scrolling horizontally on a long text, if that's what you mean yes ... it looked like working

Right now this pull request is a no go. Not before the WebView gets styling rules to look similar to what TextView based readme looks like. And I think we actually need these styling rules for the preview in comment editor so pull request improving that would be great on its own.

Totally agreed, the intention was not replacing with something uglier :P of course, but trying to see if a WebView could be used instead ;)

Because having one WebView per issue comment view totally destroys performance. Yes, we've tried
@maniac103 Did you try as well within a RecyclerView ?

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

Successfully merging this pull request may close these issues.

None yet

3 participants