-
Notifications
You must be signed in to change notification settings - Fork 5k
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
feat(fab): Initial Implementation #4598
Conversation
} | ||
|
||
fieldset .mdl-fab { | ||
margin: 16px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supernit: I think this is indented by one too many?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, looks off by two spaces. We need linting in the demo files...
b833a3e
to
b77fb33
Compare
<fieldset> | ||
<legend>Normal FABs</legend> | ||
<button class="mdl-fab"> | ||
<i class="material-icons mdl-fab__icon">favorite_border</i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this wrapper element if we just put the material-icons
class on the button and then using
.mdl-fab {
align-items: center;
justify-content: center;
}
to position the text?
Also, perhaps we should put an aria-label
attribute on the button element for accessibility purposes, since the fab only displays an icon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with making material-icons
stuff forced is what if they are using another icon set? Whether or not it is "MD". We need to be able to let developers swap out anything icon-wise, like say you wanted to use an SVG version instead.
+1 to aria-label
in the examples to help encourage using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if they are using another icon set?
Then they could just swap out the class for that icon set right?
<button class="mdl-fab material-icons" aria-label="download">file_download</button>
<!-- OR -->
<button class="mdl-fab fa fa-download" aria-label="download">
<span class="sr-only">download</span>
</button>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you meant literally move it up. Yea, that can work.
I thought you meant like, implementing the font stack right within the fab class.
4ef64f3
to
2bc638b
Compare
@@ -0,0 +1,69 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be good to have one example in the demo of a FAB positioned in the bottom-left corner, to show how easy it is to customize the behavior that way, and because it's such a common pattern in material
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, added an "edit" icon that also uses SVG to test that out. Had to add fill: currentColor
to the block to handle giving them the proper color. 😉
@Garbee did another pass on the styles. PTAL |
overflow: hidden; | ||
user-select: none; | ||
box-sizing: border-box; | ||
-webkit-appearance: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codpen of the current state as well. Shadow update is already TODO'ed in the variable file, so I'm not going to duplicate the comment again. With the typography stuff, let's also hold off just a bit until after the next sprint when we have the theme base done, as the typography should end up being more theme-bound then typography-bound. Unless we are now saying typography will be a big inter-linked dependency (which in this case would be a pretty needless one overall.) I just need to add a README and this should be good for another pass. |
0814b05
to
03b65f9
Compare
Modify to fit your designs requirements. | ||
--> | ||
<style> | ||
.app-fab--absolute.app-fab--absolute { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a repeated selector here...did you mean .mdl-fab.app-fab--absolute
? And in that case, is the compound class needed? Or is it just there for demonstrative purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the generated scripts will be injected after this, the double-class hack is to increase specificity to make sure these rules take place without !important
. Otherwise the position would get overridden and it would never get moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand that exactly...but either way because this is documentation, that implicit knowledge might confuse some people. It may be more straightforward just to use an ID
#my-fab {
position: absolute;
bottom: 1rem;
right: 1rem;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @traviskaufman's suggestion.
@traviskaufman: for a bit of extra context, repeating the class name is a nice hack to increase specificity of a CSS rule, without having to use !important
or hunt around for other things to append to the rule, like parent classes. Every repetition of the class name increments the specificity.
FABs are still missing box shadows: Resting/default shadow (6dp):
Active shadow (12dp):
|
ed7b71b
to
4fb9d7b
Compare
Just pushed the updated code and the codepen is updated with its output. Removed the |
Waiting on that PR to land before updating this one, that way we are good on the direction. Yea, should have removed the PTAL tag. |
<!-- You may also use SVG icons instead of an icon font. --> | ||
<button class="mdl-fab app-fab--absolute" aria-label="Edit"> | ||
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"> | ||
<path d="M3 17.25V21h3.75L17.81 9.94l-3.75-3.75L3 17.25zM20.71 7.04c.39-.39.39-1.02 0-1.41l-2.34-2.34c-.39-.39-1.02-.39-1.41 0l-1.83 1.83 3.75 3.75 1.83-1.83z"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this default to currentColor
or should that be added in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does default. That is the fill
CSS rule on the block.
LGTM other than the nits @traviskaufman and I pointed out. Thanks, @Garbee! |
No description provided.