refinery / refinerycms-authentication-devise

Devise based authentication extension for Refinery CMS
MIT License
17 stars 61 forks source link

New glass admin users index with[out] search #23

Closed stefanspicer closed 7 years ago

stefanspicer commented 8 years ago

This is the first admin index switched over to glass. Pages, Files, Resources to follow if this is accepted. Result looks something like this:

glass-user-index

stefanspicer commented 8 years ago

I can't seem to find the details of what test is failing. Where do I find that?

bricesanchez commented 8 years ago

@stefanspicer here: https://travis-ci.org/refinery/refinerycms-authentication-devise/jobs/142948991

stefanspicer commented 8 years ago

Okay, with https://github.com/refinery/refinerycms/pull/3211 merged, these tests now pass. Is this one good to be merged?

bricesanchez commented 8 years ago

This is my screenshot with the 2 CSS problems i've seen :

company_name_-_refinery
bricesanchez commented 8 years ago

My last concern is about icons, what about this?

stefanspicer commented 8 years ago

Here's where I stand on the icons. I don't see the problem with how I have it, and I still don't see how the other way is better. I think it will really slow me down if I have to replace all <i> tags as I merge every PR. I think the most efficient way to move forward is for someone other than me to follow up on my PRs (or work with me) and remove the <i> tags.

Here's a bit more detail on why I think we should proceed as outlined above. I just tried to remove the <i class="icon icon-add"></i> (I added the add-item_icon class to the link and defined it as .add-item_icon {@include icon('add-item')} in stylesheets/refinery/components/_icons.scss. And I got this error:

".add-item_icon" failed to @extend ".fa-add-item".
The selector ".fa-add-item" was not found.
Use "@extend .fa-add-item !optional" if the extend should be able to fail.

I have no idea what's going on there with font-awesome. It just seems overly complex. Anyway, once that is fixed, what if the icon needs to be moved up slightly? (like you just asked me to do with the trash can icon). I have to adjust padding on the icon, but not on the link text. So I guess I need a special class that only adjusts the icon. And if there are 2 icons that need to be swapped out (based on an 'active' class or something) this currently is very simple display:none css. It becomes a specific js snippet to toggle between to .icon-on and .icon-off classes I guess. And then there is the case of an icon with text centered below it. As a separate <i> tag, css has full control to move those elements around. These are what I can think of off the top of my head, but I am going to continually run into issues I assure you.

I just feel like I'm taking something simple and making it complex. I don't like to do that. I like simple. And it is well worth the cost of the extra <i> tag.

bricesanchez commented 8 years ago

I understand your point and be sure that i want to keep it simple and fast.

I could help you to fix icons and i think @anitagraham has already done a part of the job here: https://github.com/refinery/refinerycms-authentication-devise/pull/24

IMHO we introduced action_icon helper to be able to quickly update the interface across extensions if we made changes on icons core. It's a little more complex than plain HTML but easier to maintain.

I will help you to convert icons :)

stefanspicer commented 8 years ago

What updates have been made (or are foreseen) across extensions other than changing what the icon is?

I'm all for using icons like icon-add and icon-plus and icon-open that all actually use the same icon so that if we decide to change what icon icon-open is, we can just update the icon font file. And if all icon-open icons need to be larger, we can target all the i.icon-open elements and have special styling for them.

stefanspicer commented 8 years ago

My reasons above are all fairly practical. I just thought to look what others are doing. Check out the warning here: http://glyphicons.bootstrapcheatsheets.com/#collapseOne They're using an <i> tag here: http://fontawesome.io/examples/ and here http://google.github.io/material-design-icons/#using-the-icons-in-html

It seems like convention to use an <i> tag from what I can see.

bricesanchez commented 8 years ago

What updates have been made (or are foreseen) across extensions other than changing what the icon is?

I'm all for using icons like icon-add and icon-plus and icon-open that all actually use the same icon so that if we decide to change what icon icon-open is, we can just update the icon font file. And if all icon-open icons need to be larger, we can target all the i.icon-open elements and have special styling for them.

As i said, i will continue to implement action_icon helper where it's required and mainly adopted (like add/edit/delete links) after we will try to keep a good balance between good and best.

My reasons are all fairly practical. I just thought to look what others are doing. Check out the warning here: http://glyphicons.bootstrapcheatsheets.com/#collapseOne They're using an tag here: http://fontawesome.io/examples/ and here http://google.github.io/material-design-icons/#using-the-icons-in-html It seems like convention to use an tag from what I can see.

That's another discussion, more Accessibility oriented but i think <i> tag should not be used to display graphics, :before and :after pseudo css class are more aimed for this.

http://www.w3schools.com/tags/tag_i.asp