Image link should not render the external link icon by default #160
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HCRC-112. In a secondary link, an anchor element had span for text and another for the link icon. Both the sublements were displayed as flex, which made them to fill the whole row and rendering the icon in another row compared to the text.
nikomakela
force-pushed
the
HCRC-112-fix-image-link
branch
from
5556566 to
95f0270
Compare
|
For comparison: Current main:
This PR's branch:
|
HCRC-112. If there is an image included in the Link children, the external link icon should not be rendered by default for the external links. It can be forced with `showExternalIcon` prop.
nikomakela
force-pushed
the
HCRC-112-fix-image-link
branch
from
95f0270 to
d8ca726
Compare
karisal-anders
approved these changes
Jan 25, 2024
nikomakela
force-pushed
the
HCRC-112-fix-image-link
branch
from
7c6ade7 to
35213d5
Compare
karisal-anders
approved these changes
Jan 26, 2024
There was a problem hiding this comment.
It would good to document the recursiveMap function as although the code is not long it still has nuances, e.g. there are non-mapped parts (i.e. stuff not being run through fn) and mapped parts (i.e. stuff being run through fn), why the difference? What does the function really do as it skips mapping on some elements and not on others? Adding clear documentation for this would certainly help understand the code's intention.
nikomakela
force-pushed
the
HCRC-112-fix-image-link
branch
from
35213d5 to
5d4edac
Compare
HCRC-112. Add a recursively map utility, that can be used to recursively map the React children. Add a findAllElementsOfType utility, that takes advantage of the `recursiveMap` and `getChildrenByType` utilities. Add tests to each utility. Finally, the Link should use the new findAllElementsOfType utility to search for images inside an anchor.
nikomakela
force-pushed
the
HCRC-112-fix-image-link
branch
from
5d4edac to
4de2ede
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HCRC-112.
If there is an image included in the Link children, the external link icon should not be rendered by default for the external links. It can be forced with
showExternalIconprop.Also fixed the styles of the secondary link.
In a secondary link, an anchor element had span for text and another for the link icon. Both the subelements were displayed as flex, which made them to fill the whole row and rendering the icon in another row compared to the text.