Link: Fixed state bugs#3471
Conversation
|
@betrue-final-final do you want to take a look at the diffs in screener. the color has certainly changed. Also @lynamemi, are there some additional "stories" we could be testing on this component? I you haven't done it before, I'd be happy to give you a hand. |
|
I will take a look @micahgodbolt ! |
|
@micahgodbolt Pushed some stories. Let me know what you think. |
| exports[`Link renders disabled Link correctly 1`] = ` | ||
| <a | ||
| aria-disabled={true} | ||
| className="ms-Link is-disabled undefined" |
There was a problem hiding this comment.
"undefined" on the classname seems like a bug
There was a problem hiding this comment.
@lynamemi can you double check that classname and styles.root are working? would be nice to not have this undefined value here.
There was a problem hiding this comment.
I followed up with you more on Teams for extra context, but essentially, css("classname" + styles.foo) will always return "classname undefined" because of our jest config. There is a lot of precedence in fabric react for keeping it this way, but I can separate the two styles out on separate lines to get rid of undefined:
disabled && 'is-disabled
disabled && styles.disabled
- see David's comment below
| styles.root, | ||
| className, | ||
| disabled && ('is-disabled ' + styles.isDisabled) | ||
| disabled && ('is-disabled ' + styles.isDisabled), |
There was a problem hiding this comment.
if "styles.isDisabled" is undefined... what happens?
There was a problem hiding this comment.
just trying to figure out why the snapshot has "undefined" in the classname.
There was a problem hiding this comment.
per offline discussion, I'll leave this as is because it's a nuance of how the jest config treats scss
|
@micahgodbolt, I was working with @lynamemi, but when screener picks up the diff I'll take a look! |
|
@betrue-final-final I don't think screener will pick up these fixes. The styles (colors and states) where mostly right already, it was the config and our website that was buggy. And it doesn't look like there is a screener for Link: No href: disabled. Maybe we can add one? |
|
talked to Ben and just realized adding snapshots is not the same as adding stories 😄 |
|
bump for @micahgodbolt - new tests added! |
|
@micahgodbolt I think everything in this PR has been resolved - can I get an approval to get this merged in? |
|
Thanks Micah! |
4 participants
Pull request checklist
$ npm run changeDescription of changes
Before:
After:
Focus areas to test
(optional)