Skip to content
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

FLUID-6391: Updated the docs to reference Infusion-Icons #149

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jobara
Copy link
Member

@jobara jobara commented Aug 12, 2019

@jobara
Copy link
Member Author

jobara commented Aug 12, 2019

@jhung could you please review this PR?

@jobara
Copy link
Member Author

jobara commented Jun 11, 2020

@jhung could you please take another look at this PR?

@jhung jhung self-requested a review September 14, 2020 15:24
@jhung
Copy link
Member

jhung commented Sep 14, 2020

There are a couple references to IcoMoon in the documentation which are no longer needed. For example in src/documents/tutorial-iconFonts/HowToCreateAndUseFontIcons.md, lines 32, 102-118, 179.

If removing references to IcoMoon, much of the documentation about PUAs and preserving PUAs can be simplified or removed since infusion-icons makes it easier to maintain those. More specifically, lines 177-186, 189-254, 323-343 can probably be removed.

The problem here is if we replace the image in the anchor with an icon font, any text descriptions (the alt text) will
be removed as well - causing a possible usability and accessibility issue.
The problem here is if we replace the image in the anchor with an icon font, any text descriptions (the alt text)
<!-- will be removed as well - causing a possible usability and accessibility issue. -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentionally put in a comment? I think it should still be part of the text content.

Above: An image showing the Mac OS X Characters window. The Unicode value for a custom character appears in the right
most column.
* Less browser support, although modern browsers should handle most features
* Many different ways to add to a page, but only those that allow access to the SVG markup will permit styling via CSS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need to elaborate this point. Perhaps stating that embedding the SVG is the markup is the standard practice for styling SVGs with CSS.


Above: An image showing the Mac OS X Characters window. The Unicode value for a custom character appears in the right
most column.
* Less browser support, although modern browsers should handle most features
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a reference to https://caniuse.com/?search=svg

most column.
* Less browser support, although modern browsers should handle most features
* Many different ways to add to a page, but only those that allow access to the SVG markup will permit styling via CSS
* Styling multi-coloured SVGs can be complex and may not always be supported
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really necessary to say that styling SVGs with multiple colours is complex.

Can you give an example where styling an SVG with multiple colours using CSS isn't supported by a browser that supports SVGs?

@jobara
Copy link
Member Author

jobara commented Sep 15, 2020

There are a couple references to IcoMoon in the documentation which are no longer needed. For example in src/documents/tutorial-iconFonts/HowToCreateAndUseFontIcons.md, lines 32, 102-118, 179.

If removing references to IcoMoon, much of the documentation about PUAs and preserving PUAs can be simplified or removed since infusion-icons makes it easier to maintain those. More specifically, lines 177-186, 189-254, 323-343 can probably be removed.

The documentation on lines 189-254 which I believe corresponded to the "Preserving Existing Fonts" section seems to still be relevant without IcoMoon. This is to explain how to avoid conflicts with other fonts on the page. I've left it in, but changed the other locations.

@jobara
Copy link
Member Author

jobara commented Sep 15, 2020

@jhung ready for more review.

@jobara jobara changed the base branch from master to main October 22, 2020 20:52
@jobara jobara added this to the Infusion 3.0 milestone May 5, 2021
@jobara
Copy link
Member Author

jobara commented May 11, 2021

@jhung this has been updated with main and is ready for review. One thing to note is the reference to grunt which is still being used by Infusion-Icons. Should we migrate that first? Also it does seem a bit strange to have these docs as part of the Infusion Docs when it is for a package that isn't actually part of Infusion. Another options would be to migrate these docs into the Infusion-Docs repo itself or onto the wiki. This page is "popular" though, so if we do migrate it, we should leave a redirect behind.

@jobara
Copy link
Member Author

jobara commented May 12, 2021

@jhung based on the discussion in the matrix channel with @greatislander and @amb26 I'm thinking that we should not merge this PR. Maybe mothball it. Eventually migrate the existing docs to the wiki or maybe one of the guides if needed, and redirect to that location from our docs, as the page is popular. We'll also want to keep tracking the SVG icon approaches and document as needed where appropriate.

@jhung
Copy link
Member

jhung commented May 13, 2021

I'm okay with that. There are at least 3 existing wiki pages about icon fonts, SVG icons, or Icomoon. It would be good to clean or clarify the relevance of those documents as we're changing our approach.

@jobara jobara removed this from the Infusion 3.0 milestone May 13, 2021
@jobara jobara added the mothballed A PR that is indefinitely suspended, but not cancelled outright, and may resume Edit Delete label May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mothballed A PR that is indefinitely suspended, but not cancelled outright, and may resume Edit Delete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants