-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Update icons to use blade components, standardize button colors and format #15327
Conversation
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
PR Summary
|
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
'id' => false, | ||
'title' => false, | ||
]) | ||
<i {{ $attributes->merge(['class' => Icon::icon($type).' '.$class]) }} {{ isset($style) ? $attributes->merge(['style' => $style]): '' }} {{ isset($title) ? $attributes->merge(['title' => $title]): '' }} aria-hidden="true"></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.
I think, since the only place the 'Icon helper' is used is right here, that you might be able to directly paste it in above this <i>
tag, in a @php
block.
Then the thing that spits out the icons is right next to the place we use the icons, right? And you can get rid of some other extra little bits here and there.
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.
There are some other places where we're not directly generating an icon itself though (some transformers, etc) so it seemed better to keep that logic out of the blade component.
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.
In fact, you can just make it into a simple PHP array if it makes sense - something like:
$iconmap = [
/* ..... */
'caret-down' => 'fa fa-caret-down',
/* ..... */
];
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.
Again, I didn't want that logic in the blade, since there are other places where it could be useful, specifically when we're trying to determine which icon to use in transformers, even though we're not actually using that icon in those methods, we're passing it elsewhere (sometimes because it's in javascript, etc)
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.
fwiw, I'm for avoiding a giant @php
block in the component too.
"blade components" in the title caught my eye. I'm going to add myself as a reviewer to see if I have any other input 😄 |
@marcusmoore ticktock ticktock :P |
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.
'id' => false, | ||
'title' => false, | ||
]) | ||
<i {{ $attributes->merge(['class' => Icon::icon($type).' '.$class]) }} {{ isset($style) ? $attributes->merge(['style' => $style]): '' }} {{ isset($title) ? $attributes->merge(['title' => $title]): '' }} aria-hidden="true"></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.
fwiw, I'm for avoiding a giant @php
block in the component too.
'id' => false, | ||
'title' => false, | ||
]) | ||
<i {{ $attributes->merge(['class' => Icon::icon($type).' '.$class]) }} {{ isset($style) ? $attributes->merge(['style' => $style]): '' }} {{ isset($title) ? $attributes->merge(['title' => $title]): '' }} aria-hidden="true"></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.
Another note: I think we should avoid using isset
in components since that should be handled by @props
. If something is in @props
without a default value then the component will throw an exception when trying to render and if it does have a value then we don't need to use isset
(we could add a conditional around the value if needed of course).
In this case we should be able to remove class
from the props list since {{ $attributes->class(...) }}
should handle that.
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 tried that, but it still threw an error that the variable was not set.
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
This looks a lot scarier than it is, with all the files it touches. It simply switches to using an icon blade component where we can, so that we can always be sure that the icon is correct, and has the appropriate attributes (
aria-hidden
, for example).This will improve our accessibility as well, since in working on this I found a bunch of icons that didn't have the
aria-hidden
attribute, and it will (hopefully) mean less hunting down icons that we use consistently throughout the system.As I was poking around in some of the transformers, I also got rid of a stupid way we were handling titles on some of the older ones. I must have missed those when I came up with the better way (using the custom
css-blah
class.)Note: There are a few places we cannot use the icon blade components yet. I'm working on those, but some may be unsurmountable - specifically the error messages we include under each form field. It's not really a big deal IMHO, since they always use the same icon, but I bet I can probably work on that too. It breaks because we use
{!! !!}
for our error messages so that we can use the<span class="alert-msg"
to apply the red error color to the error message, but that means the component doesn't get parsed properly. Something I can work around in a different PR I think.Icons that are only used once were not included in the icon helper.
This PR also standardizes the order and color of the main action buttons (edit, checkout, checkin, etc) across all first-class views.