Social icon

[nikkuAg]

This PR is related to improving the design of social media icons on the website.
Issue: #345
I have mainly changed the design for the icons present in the footer as they were not looking very pleasant. I have also removed the border around the floating icons.

Here are the screenshot for the same:

[quozl]

Thanks.

Looking at your commit message and the files changed tab, I can't figure out if what you did was your own changes diverging from the upstream ionicon library or if you have adopted any of their changes. Which is it?

See also #353 for the problem we have with the library; we have an old version.

See Making commits in our guide to contributing; this section shows how to communicate clearly via commit messages. You have shown a tendency to use the pull request message instead of the commit messages. Remember that the pull request is eventually destroyed, and only the commit messages remain in git.

[shubhayu09]

Looking at the commits, I can see changes have been made diverging from the upstream iconic library and made his changes but it looks good, both the footer icons and floating icons. But I'm not sure about the size of the footer icons, have you changed or is it's same as before?
And also I agree with the point that commit messages are not so clear which should be clear for other users to avoid confusion and for a better understanding of the changes.

[nikkuAg]

Hey @quozl @shubhayu09 ,
Yes I have made my changes but only padding, margin and color.
The size of icons are same as before. I have just added padding and background color to have the look present now.

[quozl]

Thanks. Does updating to latest upstream make your changes any different? It would be good to be up to date.

[nikkuAg]

My branch is already up to date with upstream.

[quozl]

The upstream ion icon library?

[nikkuAg]

The upstream ion icon library?

When I am trying for fetching upstream on my fork repository there it is showing branch already up to date with upstream

[quozl]

https://github.com/ionic-team/ionicons is the ion icons repository, is that the one you are talking about?

[nikkuAg]

No, I was just using those which were available in the code. Since in the code of webiste there has been no changes for using new icons of ions-icons

[quozl]

Do your changes work with the later version of ionicons, or do they make it worse?

[nikkuAg]

See the changes I have made are basically adding border, background color, padding and margin, which I have added in the classes defined as ion-icons-facebook and others. So even if you update to latest version of ion-icons I just have to add those in the classes so that background padding border are incorporated as the icons are still the same in the new version of ion-icons.

[chimosky]

See the changes I have made are basically adding border, background color, padding and margin, which I have added in the classes defined as ion-icons-facebook and others. So even if you update to latest version of ion-icons I just have to add those in the classes so that background padding border are incorporated as the icons are still the same in the new version of ion-icons.

If your changes involve changing ion icons then you'll have to make that change upstream first if it isn't part of that already as we don't maintain ion icons and just update it when we need to so we don't want it to diverge from its upstream.

[quozl]

I don't know enough to merge this, and @nikkuAg hasn't answered my question on it, so I looked again at the Files changed tab and the commit messages for ffeb328 and 1016309.

• the commit messages don't say much, see our Guide to contributing - commits for how to write commit messages that communicate with others, both now and in future reading of them in git,
• there are changes to indentation of JavaScript source, which makes it hard to make sure that nothing was changed in that source, and I don't know why those changes were done,
• there are lines commented out instead of removed, but I don't know why,
• there are changes to css/ion_icon_customization.scss but I don't know if this is an upstream file from the ion icons project, or if it is a file that is local to our web site.

I remember doing this before. In my earlier replies I asked questions to resolve that ambiguity. I'll now try another method; I'll clone the ionicons repository, checkout the v2.0.0 tag (because that's the version in css/ionicons.min.css in our web site), and go looking for things that look the same. Result: I can't tell, and I don't know how to be sure. I know who I'll ask; the developer making the pull request, maybe they'll know. 🤦

[nikkuAg]

Hey @quozl @chimosky,
I got your point, so if I work on this issue then I think there would be no problem in merging

[chimosky]

Hey @quozl @chimosky, I got your point, so if I work on this issue then I think there would be no problem in merging

Yes if you work on the issue there would be no problem merging.

[nikkuAg]

Hey @quozl @chimosky,

Issue: #353

I have updated the ions icons to the newest version. Since the repository of ion icons have their latest version of 6.0.1, the fonts and CSS they have used in the newest version is v4.5.10.

After updating the icons version I have also corrected the CSS required for the consistency of all the icons.

[quozl]

Thanks, good progress, but a few review comments not yet resolved. Please read through the Files changed and undo any of the changes you did not intend.

[nikkuAg]

Hey @quozl,
I have viewed all the files and updated the changes so that there are no such changes which are not required.

[quozl]

Thanks.

[quozl]

Reviewed after automatic deployment; there's something odd about the instagram icon being larger than the others, but I'm no expert in social media. Several of the new files weren't fetched by Firefox, but I guess they may be browser-specific.