Foundation-sites: 6.4 Release icon-top doesn't appear to work

[hireahenchman]

How to reproduce this bug:

The icon-top doesn't appear to work. It's not working in the foundation 6.4 documentation either so I can only assume it's a bug in the release?

What should happen: Icon should be on top

What happened instead: icon is left aligned

Browser(s) and Device(s) tested on: Mac Safari & Chrome

Foundation Version(s) you are using: 6.4

[kball]

I'm seeing this as well... will put together a PR to fix and we can include it in 6.4.1, which I think will turn around later this week as we see more issues come in from the wider audience on 6.4

[kball]

Actually, looking at this now I think this might have been a change that didn't make it into the docs... it looks like we now expect the .menu to have .icons to declare some general icon styles and then an .icon-left or .icon-top to position.

My guess would be that this would be to improve our css structure & lower specificity, which was a large motivation behind the menu rework.

@brettsmason or @IamManchanda can you confirm if this is the intent so we can update the docs and add a migration note?

[codetheorist]

By looking at the code, it looks like this is the intended functionality as @kball states.

[brettsmason]

@kball Sorry for the delay. I've put together a quick CodePen based on the visual tests we have which shows them working OK. I cant remember if markup requirement has changed from 6.3 or not (did older docs versions get added anywhere in the end? I remember seeing something about it...).

Maybe the docs examples need updating based on this. It looks like its missing the icons class which you were spot on about. We could probably include all 4 examples to better showcase this.

[IamManchanda]

@codetheorist @hireahenchman Would anyone would like to submit a PR for this ??

---

BTW thanks @brettsmason for looking into it :wink:

[IamManchanda]

We could probably include all 4 examples to better showcase this.

:+1:

[codetheorist]

Had a look at this and the only available class to change the layout is .icon-top.

@IamManchanda I've just submitted the PR however, if the other layout-changing classes are not going to be implemented, would it be better to move display: -webkit-box; display: -webkit-flex; display: -ms-flexbox; display: flex; to the .icon-top class?

[codetheorist]

Ignore me, new PR coming very soon!

[IamManchanda]

if the other layout-changing classes are not going to be implemented, would it be better to move display: -webkit-box; display: -webkit-flex; display: -ms-flexbox; display: flex;

@codetheorist We just need to add display:flex;, Autoprefixer does the rest

On the question, i will be hesitant to move display: flex from icons to icon-top etc
Let update the docs with .icons.icon-top

But yes for backward compatibility, i dont think that
This bit will be bad https://codepen.io/IamManchanda/pen/vZjxJP

But please bind it with a variable where user can remove this backward compatibility in his settings file!

[codetheorist]

As mentioned, it was my own error making me believe that .icon-top was the only available layout class, this was incorrect information on my part.

I have submitted a PR for the docs with examples, etc.

There is indeed 4 layout classes available.

[kball]

Closing this based on #10331