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

IGAPP-973: Mark external links #815

Merged
merged 19 commits into from
Dec 12, 2022
Merged

IGAPP-973: Mark external links #815

merged 19 commits into from
Dec 12, 2022

Conversation

LeandraH
Copy link
Contributor

This pull request belongs to the issue https://issues.tuerantuer.org/browse/IGAPP-973.

To test on web: Go to http://localhost:9000/testumgebung/de/test-f%C3%BCr-externe-links, look at the links
To test on native: Go to Testumgebung, in German, click on Test für externe Links and look at the links.

Co-authored-by: charludo <47758554+charludo@users.noreply.github.com>
Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

I have some rendering problems with the icon underline under the icon with chrome, check screenshot. This can be fixed by using display:flex for link and after-Element but this may cause other errors, so pls check before

image

LeandraH and others added 3 commits November 16, 2022 11:27
Co-authored-by: Andreas Fischer <37902063+f1sh1918@users.noreply.github.com>
Co-authored-by: Andreas Fischer <37902063+f1sh1918@users.noreply.github.com>
@LeandraH
Copy link
Contributor Author

Okay, I tested it on OS X Firefox, Chrome, Safari, and iOS and Android simulations, and it all looks good there. Would love for someone with Windows to test there a little :)
The weird underline didn't actually happen in native but I think it's better to still give it the display flex, just in case it changes some behavior down the line.

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Nicely done!

Do you think it would be possible to also implement this for the NativeHtml component in native? Otherwise this does not work in the list content, see screenshot:

native/src/utils/renderHtml.ts Outdated Show resolved Hide resolved
native/src/utils/renderHtml.ts Show resolved Hide resolved
web/src/components/RemoteContent.tsx Show resolved Hide resolved
Co-authored-by: Steffen Kleinle <steffen.kleinle@mailbox.org>
@LeandraH
Copy link
Contributor Author

Nicely done!

Do you think it would be possible to also implement this for the NativeHtml component in native? Otherwise this does not work in the list content, see screenshot:

I'll give it a try :)

@LeandraH
Copy link
Contributor Author

Looks like react-native-render-html can't handle a <path> element so getting the icon into the NativeHtml component seems to be a no in the foreseeable future :( Looking at the speed difference, I don't think it's worth it to replace react-native-render-html for such a small feature.

@f1sh1918
Copy link
Contributor

Looks like react-native-render-html can't handle a <path> element so getting the icon into the NativeHtml component seems to be a no in the foreseeable future :( Looking at the speed difference, I don't think it's worth it to replace react-native-render-html for such a small feature.

Indeed the only solution i see is to create a logic to inject <img> or <svg> tags in this particular cases with the icon (before </a closes. but thats very hacky and can lead to errors. Not sure if thats worth to do

@steffenkleinle
Copy link
Member

steffenkleinle commented Dec 1, 2022

Is it somehow possible to use png or some other image format? Don't know much about that so no idea.

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

If it is currently not possible to use svgs in react-native-render-html could you perhaps open an issue there and in our jira to fix this later?

@LeandraH
Copy link
Contributor Author

LeandraH commented Dec 7, 2022

I got it! I can't get the space between the link and the icon to not be underlined (not even with important!) but I think that is a manageable difference.
image

Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Really nice work :) thx for the effort you put in. Works nice.
Just added the little missing piece, maybe it way too obvious :)

native/src/components/NativeHtml.tsx Outdated Show resolved Hide resolved
Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Tested on android, thanks for investing the time to find a solution!

Maybe you could write some tests for this :)

One more thing: For me the icons look quite big and I think it could be a little smaller. Did you follow a design? Since I am not an UI/UX expert you may also just ignore this if you think its better this way or this was like this in a design.

native/src/components/NativeHtml.tsx Outdated Show resolved Hide resolved
native/src/components/NativeHtml.tsx Outdated Show resolved Hide resolved
native/src/components/NativeHtml.tsx Outdated Show resolved Hide resolved
@LeandraH LeandraH merged commit ef1399a into main Dec 12, 2022
@LeandraH LeandraH deleted the IGAPP-973-Mark-external-links branch December 12, 2022 18:28
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

4 participants