Skip to content

Dubunin/breadcrumb issues #12

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
dubunin merged 8 commits into from
Jul 28, 2016
Merged

Dubunin/breadcrumb issues #12

Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
85f603d
#191731: updated hover state color for overflow button
Jul 22, 2016
c890a9d
#191719: added override styles for breadcrumb focus rect
Jul 22, 2016
0879ad6
#191719: code formatting - missing space
Jul 22, 2016
9f3323b
#191723: per design - removed negative margin-right from overflow bre…
Jul 26, 2016
4b4c609
#191723: code cleanup
Jul 27, 2016
971fc26
#191723: updated to use localized margins
Jul 27, 2016
bb8ac2c
Merge branch 'master' into dubunin/breadcrumb-issues
Jul 28, 2016
8c15bdc
191731: removed overrides that are no longer necessary. Updated to us…
Jul 28, 2016
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
25 changes: 23 additions & 2 deletions src/components/Breadcrumb/Breadcrumb.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,31 @@
// TODO: Overrides that need to be removed.

Copy link
Contributor

@cliffkoh cliffkoh Jul 27, 2016

Choose a reason for hiding this comment

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

I think the POR is to not have/use components CSS, you should port everything over instead and make it RTL aware

Copy link
Contributor Author

@dubunin dubunin Jul 27, 2016

Choose a reason for hiding this comment

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

@cliffkoh can you provide a little more info here? Not quire sure what this comment is about and/or how to act on. Thanks.

Copy link
Contributor

@cliffkoh cliffkoh Jul 28, 2016

Choose a reason for hiding this comment

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

@dubunin Basically you shouldn't have @import '~office-ui-fabric/src/components/Breadcrumb/Breadcrumb';.

I am frankly surprised there isn't a merge conflict, can you check what is in master?

Also tagging @dzearing

.ms-Breadcrumb-overflow {
@include margin-right(0px);
Copy link
Contributor

@cliffkoh cliffkoh Jul 28, 2016

Choose a reason for hiding this comment

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

You shouldn’t be having overrides anymore since the style you’re overwriting is just above in the very same file…

display: inline-block;
}

.ms-Breadcrumb-overflowButton {
@include margin-right(0px);
line-height: 12px;
margin-right: 0px;
}

// Temp fix. Should be updated in fabric core
Copy link
Member

@Jahnp Jahnp Jul 26, 2016

Choose a reason for hiding this comment

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

Not necessarily true anymore--you can probably remove this todo.

&:hover {
color: $ms-color-themeDark;
}
}

// Override the default fabric styles for focus which do not make use of FocusZone
.ms-Breadcrumb-itemLink {
&:focus {
outline: none;
color: $ms-color-neutralPrimary;
}
}

.ms-Fabric.is-focusVisible .ms-Breadcrumb-itemLink {
&:focus {
outline: 1px solid $ms-color-neutralSecondaryAlt;
color: $ms-color-black;
}
}