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

[Tag] Improve outline behavior #2428

Merged
merged 10 commits into from
May 16, 2018
Merged

[Tag] Improve outline behavior #2428

merged 10 commits into from
May 16, 2018

Conversation

llorca
Copy link
Contributor

@llorca llorca commented Apr 27, 2018

  • Add tabIndex on interactive tags only, for proper keyboard navigation
  • Clean up focus styles

@llorca llorca requested a review from giladgray April 27, 2018 18:48
@blueprint-bot
Copy link

add tabIndex for focus on interactive tags

Preview: documentation | landing | table

@giladgray
Copy link
Contributor

@llorca TagInput backspace & arrow key interactions look broken -- no focus state

@@ -5,10 +5,14 @@
@import "../forms/common";
@import "../tag/common";

$ti-padding: ($pt-input-height - $tag-height) / 2;
$tag-input-padding: ($pt-input-height - $tag-height) / 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was a bit confused by the namespace look of $ti so named this more classicly... if you feel strongly, I can revert back, but this seems more legible to me

@llorca
Copy link
Contributor Author

llorca commented Apr 27, 2018

@giladgray added a custom outline style for tag input tags only. let me know if that's a fair approach, otherwise could use some help figuring out the best way here

@blueprint-bot
Copy link

add custom outline styles for tag input tags

Preview: documentation | landing | table

// NOTE: in order to wrap long words, you must set explicit width on TagInput,
// or use .#{$ns}-fill CSS class modifier.
overflow-wrap: break-word;

&::before {
Copy link
Contributor

Choose a reason for hiding this comment

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

not a fan of this special case, and even less a fan of not simply using outline / the default focus styles (which it used to before this PR)

@blueprint-bot
Copy link

use focus outline mixin instead of custom box shadow

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix view source margin bottom by adding a heading

Preview: documentation | landing | table

@@ -88,7 +88,7 @@ export class Tag extends React.PureComponent<ITagProps, {}> {
) : null;

return (
<span {...htmlProps} className={tagClasses}>
<span {...htmlProps} className={tagClasses} tabIndex={interactive ? 0 : -1}>
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we want undefined in the false case (emphasis mine):

A negative value (usually tabindex="-1") means that the element should be focusable, but should not be reachable via sequential keyboard navigation. Mostly useful to create accessible widgets with JavaScript.
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex

}
}

.#{$ns}-tag {
position: relative;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think this chance is necessary. leftover from previous attempt?

// NOTE: in order to wrap long words, you must set explicit width on TagInput,
// or use .#{$ns}-fill CSS class modifier.
overflow-wrap: break-word;

&.#{$ns}-active {
@include focus-outline(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

i kinda think we want to adjust outline offset on all tags, not just in tag inputs:

image
(note how the outline touches the neighboring tags)

image

so this change should move back to core tag styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's the right move, but did it anyway, Not sure what to do with outline design in general

@giladgray
Copy link
Contributor

@llorca please merge latest master for the new build steps

@llorca llorca requested a review from giladgray May 8, 2018 21:31
@blueprint-bot
Copy link

address comments

Preview: documentation | landing | table

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

a few more changes. i pushed a commit, so be sure to pull first

$tag-input-icon-padding: ($pt-input-height - $pt-icon-size-standard) / 2;
$tag-input-icon-padding-large: ($pt-input-height-large - $pt-icon-size-large) / 2;

$tag-input-outline-color: $pt-outline-color;
Copy link
Contributor

@giladgray giladgray May 8, 2018

Choose a reason for hiding this comment

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

remove this alias, use original name directly also unused, please delete.


$tag-input-outline-color: $pt-outline-color;
$tag-input-outline-offset: 1px;
$tag-input-outline-size: 2px;
Copy link
Contributor

Choose a reason for hiding this comment

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

these two vars are unused; please remove

@@ -120,6 +121,7 @@ $tag-padding-large: ($tag-height-large - $pt-icon-size-large) / 2 !default;
background-color: rgba($background-color, $opacity + 0.1);
}

&.pt-active,
Copy link
Contributor

Choose a reason for hiding this comment

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

#{$ns}

@@ -106,6 +106,7 @@ $tag-padding-large: ($tag-height-large - $pt-icon-size-large) / 2 !default;
background-color: rgba($background-color, $opacity - 0.15);
}

&.pt-active,
Copy link
Contributor

Choose a reason for hiding this comment

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

pt#{$ns}

$ti-icon-padding: ($pt-input-height - $pt-icon-size-standard) / 2;
$ti-icon-padding-large: ($pt-input-height-large - $pt-icon-size-large) / 2;
$tag-input-icon-padding: ($pt-input-height - $pt-icon-size-standard) / 2;
$tag-input-icon-padding-large: ($pt-input-height-large - $pt-icon-size-large) / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add !default to all these (surviving) vars please?

@blueprint-bot
Copy link

use tabIndex from props if given

Preview: documentation | landing | table

@llorca llorca requested a review from giladgray May 8, 2018 22:19
@blueprint-bot
Copy link

Merge branch 'al/tag-outline' of github.com:palantir/blueprint into al/tag-outline

Preview: documentation | landing | table

@@ -27,6 +27,10 @@ $tag-padding-large: ($tag-height-large - $pt-icon-size-large) / 2 !default;
color: $pt-dark-text-color;
font-size: $pt-font-size-small;

&.#{$ns}-active {
Copy link
Contributor

Choose a reason for hiding this comment

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

ah dang actually i think this should be &:focus here, just to adjust the outline-offset.

and bring back this selector in .pt-tag-input .pt-tag like you had earlier

😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed another commit, hopefully I got it right this time!

@blueprint-bot
Copy link

address more comments

Preview: documentation | landing | table

@llorca llorca requested a review from giladgray May 16, 2018 23:17
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