NGSTACK-452 - unify external link/internal relation into single macro #40
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
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.
In order to generate the URL on the ng_banner content types, the repetitive chunk of code was used which first checked if external URL value is set or either an internal object relation is set in order to set a link value. It was repeated across all macros and was sometimes repeated in other twig templates as well, with no possibility of "unifing" the logic in one separate macro and reusing it when needed.
A link_generator macro was created in order to make this possible. Now, other macros are much more lightweight and link_generator macro can be used in other templates as well, if needed.
This change was tested on four different cases: if banner does not have neither relation or URL, if banner has relation, if banner has URL and if banner has both relation and URL set.
ISSUES: since link_generator macro is used only to set an URL value, it does not have any additional logic like checking if "open_in_new_window" is set, and this logic is left in existing macros. This made a change that interal relation value can also be opened in new tab, which can be considered a feature since this use cases have been required in the past. The only issue that has been identified as an "unwanted" behaviour is the case where neither relation or URL have been set but checkbox "open_in_new_window" is set, which generates an image link to # in new tab, however, this will be resolved in another PR where ng_banner will be modifed that it can work without link markup set (the issue with explicit CSS rules on banner link).