Skip to content

Textfield: Fixed font bugs #1663

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.

Sign up for GitHub

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

Merged
dzearing merged 6 commits into from
May 3, 2017
Merged

Textfield: Fixed font bugs #1663

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b2a942
Fixed input font
micahgodbolt Apr 28, 2017
fab6901
Fixed textfield bugs, fonts, focus
micahgodbolt May 1, 2017
c079a4b
Change file
micahgodbolt May 1, 2017
a54f58e
Merge branch 'master' into textfield-text-fixes
micahgodbolt May 1, 2017
451b326
Merge branch 'master' into textfield-text-fixes
micahgodbolt May 2, 2017
d2a9d25
Merge branch 'master' into textfield-text-fixes
cschleiden May 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions common/changes/textfield-text-fixes_2017-05-01-15-56.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "TextField: Fixed bugs in textfield font family and focus borders",
"type": "patch"
}
],
"email": "micahgodbolt@gmail.com"
}
24 changes: 10 additions & 14 deletions packages/office-ui-fabric-react/src/components/TextField/TextField.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@
border-color: $ms-color-neutralSecondaryAlt;
}

&:focus {
&.fieldGroupIsFocused {
border-color: $ms-color-themePrimary;
}
&:hover,
&:focus {
&.fieldGroupIsFocused {
@media screen and (-ms-high-contrast: active) {
border-color: $ms-color-contrastBlackSelected;
}
Expand All @@ -116,14 +116,20 @@
}

.field {
@include ms-u-normalize;
@include ms-baseFont;
border-radius: 0;
border: none;
color: $ms-color-neutralPrimary;
@include padding(0, 12px, 0, 12px);
width: 100%;
outline: 0;
text-overflow: ellipsis;

outline: 0;
Copy link
Copy Markdown
Contributor

@phkuo phkuo May 1, 2017

Choose a reason for hiding this comment

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

is this outline:0 necessary?

Copy link
Copy Markdown
Member Author

@micahgodbolt micahgodbolt May 1, 2017

Choose a reason for hiding this comment

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

I'm not adding it, just moved it from 124.

&:active,
&:focus,
&:hover {
outline: 0;
}
&.hasIcon {
@include padding-right(24px);
}
Expand Down Expand Up @@ -169,11 +175,6 @@
}
}

&:active,
&:focus {
border-color: $ms-color-themePrimary;
}

:global(.ms-Label) {
font-size: $ms-font-size-m;
@include margin-right(8px);
Expand All @@ -186,11 +187,6 @@
flex: 1 1 0;
border: 0;
@include text-align(left);
&:active,
&:focus,
&:hover {
outline: 0;
}
}

&.rootIsDisabled {
Expand Down
2 changes: 1 addition & 1 deletion packages/office-ui-fabric-react/src/components/TextField/TextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ export class TextField extends BaseComponent<ITextFieldProps, ITextFieldState> i
return (
<div className={ textFieldClassName }>
{ label && <Label htmlFor={ this._id }>{ label }</Label> }
<div className={ css(styles.fieldGroup) }>
<div className={ css(styles.fieldGroup, isFocused && styles.fieldGroupIsFocused) }>
Copy link
Copy Markdown
Contributor

@cschleiden cschleiden May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

css(styles.fieldGroup, {
  [styles.fieldGroupIsFocused]: isFocused
}

? but probably doesn't matter

Copy link
Copy Markdown
Member Author

@micahgodbolt micahgodbolt May 2, 2017

Choose a reason for hiding this comment

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

@cschleiden i believe we're actually trying to move away from that notation for compatibility with Glamor.

Copy link
Copy Markdown
Contributor

@cschleiden cschleiden May 2, 2017

Choose a reason for hiding this comment

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

Really? It's so much nicer than the other one :) @dzearing ?

Copy link
Copy Markdown
Member

@dzearing dzearing May 2, 2017

Choose a reason for hiding this comment

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

I would suggest seperating out separate lines for each rule, but yes, I'd like to move away from the object notation, for example:

<div 
  className={ 
    css(
      styles.fieldGroup,
      isFocused && styles.fieldGroupIsFocused
    ) }
/>

FWIW: when we introduce glamor, we need to normalize what the css helper does. In glamor, it can take objects that hold styles:

css({
   background: red
 })

whereas ours takes strings. I'd love a helper that can take both:

css(
  'myStringClassName',
  { background: 'red' }
)

Which should return a string myStringClassName css-123abc.

Copy link
Copy Markdown
Member

@dzearing dzearing May 2, 2017

Choose a reason for hiding this comment

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

The object map notation is kind of nice, but really it is weird that we have 2 different ways to do the same thing and it would be nice to be consistent. If we can move away from the object maps, that opens the door to using the new helper. I will need to call it something other than css to differentiate the behavior.

{ (addonString !== undefined || this.props.onRenderAddon) && (
<div className={ css(styles.fieldAddon) }>
{ onRenderAddon(this.props, this._onRenderAddon) }
Expand Down