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

<KExternalLink> final fixes for icons and margins #143

Merged
merged 5 commits into from
Feb 19, 2021

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Jan 5, 2021

@nucleogenesis - it's all fixed now! Here is a summary of what was updated:

In either LTR or RTL languages, the direction of the icon matches the language of the text of the link

English text in English Arabic text in Arabic
Screen Shot 2021-01-05 at 9 57 34 AM Screen Shot 2021-01-05 at 1 35 25 PM
Now: English text in Arabic (Previously: English text in RTL)
Screen Shot 2021-01-05 at 1 06 08 PM Screen Shot 2021-01-05 at 9 56 58 AM

If the user does not choose to open in a new tab, then the link has no left margin.

No margins, no icon - English No margins, no icon - Arabic
Screen Shot 2021-01-05 at 9 53 19 AM Screen Shot 2021-01-05 at 9 53 48 AM

If the user uses openInNewTab, icon appears correctly and has no left margin.

No margins, with icon - English No margins, with icon - Arabic
Screen Shot 2021-01-05 at 9 51 56 AM Screen Shot 2021-01-05 at 9 52 28 AM

@sairina sairina requested a review from nucleogenesis January 5, 2021 21:42
lib/buttons-and-links/KExternalLink.vue Outdated Show resolved Hide resolved
@sairina
Copy link
Contributor Author

sairina commented Feb 4, 2021

Update

I realized I had deleted the icon and iconAfter props completely (instead of just adding the openInNewTab prop). So, I restored that functionality and double-checked that external page icons still appear correctly. See images below. Sorry about that!

Arabic icon Arabic iconAfter
Screen Shot 2021-02-03 at 9 18 39 PM Screen Shot 2021-02-03 at 9 18 26 PM
English icon English iconAfter
Screen Shot 2021-02-03 at 9 21 59 PM Screen Shot 2021-02-03 at 9 21 33 PM

@sairina sairina requested a review from nucleogenesis February 4, 2021 05:49
@sairina sairina dismissed nucleogenesis’s stale review February 16, 2021 20:39

I think this was addressed after some changes to the code - if not, I can take a look at it again!

@indirectlylit
Copy link
Contributor

In both chromium and firefox on mac I see this:

image

for

        <KExternalLink
          text="external (google)"
          href="https://google.com"
          icon="facility"
          iconAfter="facility"
        />

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

merging in so we can see everything together. Will file follow-up for cross-component consistency

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.

3 participants