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

Add purple clickable icons for KRouterLink #150

Merged
merged 5 commits into from
Feb 19, 2021

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Feb 9, 2021

Summary

Updated KIcon in KRouterLink so that KRouterLink now has icon and iconAfter props.

Addresses

Issue #135

Screenshots After

Examples of icon placement for icon and iconAfter props:

For LTR For RTL
Screen Shot 2021-02-08 at 9 22 11 PM Screen Shot 2021-02-08 at 9 37 50 PM

Screenshots Before

Screen Shot 2021-02-03 at 10 03 33 PM

@sairina sairina marked this pull request as ready for review February 9, 2021 18:20
@indirectlylit
Copy link
Contributor

#103 seems related?

@sairina
Copy link
Contributor Author

sairina commented Feb 10, 2021

Hmm, I'm not sure if this is related to this particular PR because I didn't work on any code around KButton for this, but I know this is one of the components that were mentioned that should all have similar behavior, so I'd be happy to take a look at this issue after this one is closed out!

Speaking of which, @indirectlylit , can you take a look at the updated KRouterLink code (specifically for the icon) - I made sure to fix the slot issue that had brought up the 1px red border. It turns out that because all of those icon slotting issues were taken care of from KLabeledIcon, there was no need to add a computed class to this component.

Before After
Screen Shot 2021-02-10 at 2 34 29 PM Screen Shot 2021-02-10 at 2 32 05 PM

@sairina sairina marked this pull request as draft February 10, 2021 22:56
@sairina sairina marked this pull request as ready for review February 10, 2021 23:09
@indirectlylit
Copy link
Contributor

indirectlylit commented Feb 19, 2021

In chromium and firefox on mac I see this:

image

for:

        <KRouterLink
          text="router link (empty)"
          :to="{}"
          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.

2 participants