search

dhis2 / notes

:memo: Memos, proposals, agendas and meeting minutes
19 stars 8 forks source link

Icon component api requirements #163

Closed ismay closed 3 years ago

ismay commented 4 years ago

Branching off of https://github.com/dhis2/ui/pull/284 to discuss the api requirements for our icon components.

ismay commented 4 years ago

Requirements that I'm aware of:

There are 120 icons total (at the moment, judging from the screenshot in the linked PR). Rough size of all svgs together is 169 kB, gzipped around 39.9 kB (if they'd all be included, unminified). Maybe we should first discuss our ideal api, and then talk about optimisations for size, tree shaking, etc. And then in a different issue we can tackle the build process.

ismay commented 4 years ago

Things that aren't clear to me:

Also, are there any requirements missing in the comment above?

cooper-joe commented 4 years ago

@cooper-joe mentioned: This is to allow flexibility in different types.. So if I understand correctly, you would like the flexibility to add to or change the icon types over time, like filled, bold, etc., right?

Yes, I think is important to avoid rewrites in the future when unforeseen changes require different icon types. I don't want the architecture of the ui-icons library to be the reason that one-off icon types aren't added, for example.

As a follow up to the previous question, do you think that the icon types will always be mutually exclusive (either bold or filled, but never bold and filled)?

I think it is possible that icon types might not be mutually exclusive.

Also, are there any requirements missing in the comment above?

Possibly a default color, but I'm not decided on this yet.

However, I would argue that "it should be possible to set variants" should be removed. As noted above, I don't think we commit to any set of variants, or whether these variants might need to be used together. To me it is simpler and more flexible to remove the concept of variants from the api and instead use separate icons.

So instead of:

<IconMail/>
<IconMail type="bold"/>
<IconMail type="filled"/>

it would be:

<IconMail/>
<IconMailBold/>
<IconMailFilled/>

I realize this increases the number of components in this library, but I think it is worth it to remove the prop/variant management complexity.

ismay commented 3 years ago

Yes, I think is important to avoid rewrites in the future when unforeseen changes require different icon types. I don't want the architecture of the ui-icons library to be the reason that one-off icon types aren't added, for example.

Makes sense, I agree with that. So the way I see it, allowing for (unexpected) changes is always something we (should) have in mind when building. Within reason we can try to anticipate future changes and try not to paint ourselves into a corner. On the other hand, we're only sure about our current requirements, so I think we should just try to stick to a sensible, reasonably future-proof api given our current expectations, and other than that we should be good.

Possibly a default color, but I'm not decided on this yet.

That shouldn't be a problem. If we accept a color prop, we can also set a default for it for example.

However, I would argue that "it should be possible to set variants" should be removed.

What I meant here was that there should be a mechanism for users to choose whether they want a bold icon, or a filled icon, or any other type of icon. But I didn't want to already state how we want to allow the user to do that. That's more what I meant there. So the requirement, without stating any preferred solution.

So including it in the component name is a way to allow the user to do that for example, or a flag would be a way, or a prop that accepts a string. To me, including the type in the component name seems unconventional. Whereas using a prop would seem like the more common solution to me. I'm not exactly sure why you prefer using the component name for setting the type, could you elaborate on that? The way I see it, the complexities are the same, but one is just a slightly more unconventional api.

I think it is possible that icon types might not be mutually exclusive.

Maybe we should discuss this a bit more. Exclusive types warrant a different api than non-exclusive types in my opinion. Maybe we should take a look at how other libraries have solved this.

cooper-joe commented 3 years ago

So including it [icon variant] in the component name is a way to allow the user to do that for example, or a flag would be a way, or a prop that accepts a string. To me, including the type in the component name seems unconventional. Whereas using a prop would seem like the more common solution to me. I'm not exactly sure why you prefer using the component name for setting the type, could you elaborate on that? The way I see it, the complexities are the same, but one is just a slightly more unconventional api.

My preference for including any variant in the name is, I think, based on a concern that a too restrictive api would slow down or complicate the process of adding new icons in new variants. I think I was focusing on potential bottlenecks in the process: an app needs an updated icon but can't get it until the api is updated to accept a new prop type or something, and this can't be done because we have a big release that's pending ... something similar was the theoretical situation in my head.

However, I've not given too much thought to how the build process could solve this. For example, we could automatically build variant props from filename patterns: icon-name--variant would allow as many variants as we need without manually updating the api.

So I'm not set on the variant-in-name approach, I think I prefer it because it seemed to me to be the easiest to update in the future based on uncertain requirements.

I think it is possible that icon types might not be mutually exclusive.

Maybe we should discuss this a bit more. Exclusive types warrant a different api than non-exclusive types in my opinion. Maybe we should take a look at how other libraries have solved this.

This seems like another positive for variant-in-name. IconMail IconMailBold IconMailRTL IconMailBoldRTL all work, although of course the order is completely arbitrary and hard to guess. (Edit: I realize that's the same as having single props Bold RTL and BoldRTL) As mentioned I think it is possible, but somewhat unlikely that types/variants might not be mutually exclusive. If it would be a headache for the api then perhaps we draw the line and say "we don't need this now and we don't anticipate it being a critical requirement in the future, let's shelve it".

ismay commented 3 years ago

My preference for including any variant in the name is, I think, based on a concern that a too restrictive api would slow down or complicate the process of adding new icons in new variants.

I see what you mean. So translating that to a requirement could maybe be something like:

Does that cover it you think?

As mentioned I think it is possible, but somewhat unlikely that types/variants might not be mutually exclusive. If it would be a headache for the api then perhaps we draw the line and say "we don't need this now and we don't anticipate it being a critical requirement in the future, let's shelve it".

Ok. We could just leave it open, and see whether a potential build process could allow for both exclusive and non-exclusive icons. Unless it turns out to become confusing. So I guess then we could just leave the requirement, it should be possible to set variants, filled, bold, etc., as is.

So adding the requirement from the first point, that leaves me with these requirements:

Anything missing? If not, we could go with this, finalize the icon size issue and start work on the build process. (I've posted this issue to the UI channel, so that others have a chance to participate as well before we finalize the requirements)

cooper-joe commented 3 years ago

So adding the requirement from the first point, that leaves me with these requirements:

* it should be possible to choose the icon (of course)

* it should be possible to set the icon color

* it should be possible to set the icon size

* it should be possible to set variants, filled, bold, etc.

* adding new icons, in new variants or sizes, should be as easy as possible, to prevent any slowdowns in releasing new icons

Anything missing? If not, we could go with this, finalize the icon size issue and start work on the build process. (I've posted this issue to the UI channel, so that others have a chance to participate as well before we finalize the requirements)

These requirements look good to me, thanks Ismay 👍

HendrikThePendric commented 3 years ago

Thanks @ismay I agree with the requirements as stated above.

Suggested requirements addition

There was one more point in the conversation above that stood out to me and wasn't really included explicitly in the current requirement list:

we should just try to stick to a sensible, reasonably future-proof api given our current expectations

I've emphasised future-proof here because I think it is important to produce an API for the icon components that will enable us to gradually grow the component set without producing breaking changes. I don't think this is a new requirement, it is probably implicitly part of the last one, but I would rather see it mentioned explicitly like so:

  • adding new icons, in new variants or sizes, should not produce any breaking changes and be as easy as possible, to prevent any slowdowns in releasing new icons.

Requirements with an obvious implementation strategy

I think that for some of the requirements it is fairly obvious how we should implement them:

  • it should be possible to choose the icon (of course)

Via component name

  • it should be possible to set the icon color

Via color prop

  • it should be possible to set the icon size

Via size prop (see dhis2/notes#162)

I personally think we don't have to discuss these requirements anymore, doing things like outlined above seems like a no-brainer (but feel free to disagree).

Requirements that need further discussion

It is not entirely clear to me how we should address the remaining two requirements:

  • it should be possible to set variants, filled, bold, etc.
  • adding new icons, in new variants or sizes, should not produce any breaking changes and be as easy as possible, to prevent any slowdowns in releasing new icons.

There is a clear tension between an ideal consistent API and practical considerations. To me, hypothetically, the ideal props API for our icon-library would look like this:

However, to me this would only make sense if every (or at least the vast majority) of the icons were in fact available in every size and variant. The problem is that there are a lot of permutation there, which would mean a huge number of SVGs would be needed: svgTotal = iconCount * availableSizesCount * variantCount. Currently we already have 120 icons and 2 sizes. Suppose we would want to have 3 variants, we'd already need 720 .svg files.....

@cooper-joe has already hinted at the fact that producing SVGs for this icon library is not something he's going to be able to dedicate too much time on. As such, we should expect that the set of .svg files is going to grow very gradually, and organically. Quite likely we'll see a certain icon being introduced in a single size, and single variant. Perhaps the other sizes and variants are going to be introduced much later, or perhaps never....

If we end up with a set of .svg files like that, and we would implement the props API as I outlined above, we'd end up with a very confusing situation for developers: Every icon-component is going to be slightly different in terms of what prop-values it can accept. Sure we can help by setting the correct prop-types, so the developer gets notified when using an unsupported prop or prop-value. And we can generate availability readme-table or demo-story to show which "flavours are available" for each icon. But still the core problem remains: the number of created .svgs will be much smaller than the number icon-permutations.

If we were to remove the variant prop from the icon's prop-api, and treat each icon-variant as an individual component, as @cooper-joe suggested, we could sidestep the problem above to an extend. If we need an outlined variant of the Add component and introduce this as AddOutlined, then:

I'm aware that this solution also comes with certain downsides:

On balance, my preference would be to go with the "variant is included in component name" solution, because:

ismay commented 3 years ago

adding new icons, in new variants or sizes, should not produce any breaking changes and be as easy as possible, to prevent any slowdowns in releasing new icons.

Sounds good 👍. As a goal that seems reasonable.

Requirements with an obvious implementation strategy

I've tried to keep implementation separate from the requirements in this issue on purpose, but I think it's likely that we'll end up with what you're listing. I just didn't want to already have a solution before we've clearly defined the requirements. As long as we've considered all the options I also think we'll not have any issues with these requirements.

Requirements that need further discussion. There is a clear tension between an ideal consistent API and practical considerations.

Yeah I agree. The way I see the requirements is as our guidelines. If it turns out we can't meet them, we can discuss them again and modify them. Most likely implementing it all will have us modifying the requirements a bit to fit what's possible.

On balance, my preference would be to go with the "variant is included in component name" solution

I think I've come to a similar conclusion (for now, I still have to research it properly), though my reasons are a little different than yours. The way I see it, whether we have the variant in the name, or as a prop, it's still not unlikely that a user will assume that all icons have the same variants, sizes, etc.. I think what we're going to have to do, regardless of which api we go with is

The reason that I'm also thinking towards having more elaborate icon names (with variants in there, etc.), is because it seems to align well with the existing tooling that CRA ships: https://medium.com/better-programming/react-best-way-of-importing-svg-the-how-and-why-f7c968272dd9. There are several ways of using that tooling, but it's all basically built on https://github.com/gregberge/svgr. I'm preempting the build discussion slightly, but that's my reason for leaning in this direction. Anyway, let's discuss that later.

So updating the requirements with what you've mentioned, we have:

@HendrikThePendric Let me know what you think of the updated requirements (if I've missed anything, worded it wrong, etc.). If you agree and noone has any objections I think we can create an issue for discussing the build.

HendrikThePendric commented 3 years ago

Fully agree with the requirements above 👍 . Before we close this issue and move on to discussing the build we should get input from more people, perhaps @amcgee ?

amcgee commented 3 years ago

Great discussion @ismay @cooper-joe @HendrikThePendric, thanks for the write-up!

  • it should be possible to choose the icon (of course)
  • it should be possible to set the icon color
  • it should be possible to set the icon size
  • it should be possible to set variants, filled, bold, etc.
  • adding new icons, in new variants or sizes, should not produce any breaking changes and be as easy as possible, to prevent any slowdowns in releasing new icons.
  • it should be as easy as possible for developers to discover the available icons (so no fail states that can only be discovered at run-time for example)

I agree with all these requirements, it's a nice list. A few minor additions:

  1. To the last point - developers should also be able to find "base" icons and their available variants, rather than needing to search through a list of all icons with all their variants. A decent example (although already a bit too complex imho) for reference about discoverability of in size, color, and variant is FontAwesome
  2. An additional requirement - The Icons API should facilitate the exclusion of unused icons from the final application bundle, and dynamic loading of minimal icon sets should (possibly) be supported. For me this is the main argument for including variants in the component name, rather than as a prop - tree-shaking just works. We lose that automatic tree-shaking ability for prop-based variation (like size), so we also need to consider the impact on bundle size of these API decisions.
  3. An additional requirement - invalid icon props which cannot be caught statically at compile-time should FAIL SAFE to the default icon variant - i.e. if you select a size which doesn't exist, the default size (or closest size?) should be used, rather than including nothing or a broken image tag

I think there's an aspect of variants that we're actually missing here, which is that there's a difference between static and dynamic variants. For example - a developer can pick whether or not to us a filled variant of an icon and freeze that choice at compile-time. However, a variant like RTL can't (in most cases) be known until run-time, based on the locale of the logged-in user. There may be other dynamic variants, but RTL is the most obvious one to me (and might be the only one we need to consider)

As such, we shouldn't include RTL or other dynamic variation in the component name, otherwise we'll end up with user.locale.isRTL ? <IconRTL> : <Icon> sprinkled everywhere we use every singe icon. Practically that probably won't happen and so we will end up with either partial or non-existent RTL support even if we have RTL icons in the library.

A slightly better approach, from an API perspective, would be to take a boolean rtl prop i.e. <Icon rtl={user.locale.isRTL} />, but I actually still don't think that's good enough. What if the component or app author just forgets to pass this rtl prop? For this reason, in the case of rtl at least, I think automatic support for rtl variation would make sense, meaning rtl should be excluded from the API completely - the Icon wrapper component should determine whether to load ltr or rtl variant based on the locale of the current user as determined from the App Runtime context.

amcgee commented 3 years ago

Expanding on the bundle size challenge - if we have, say, 3 sizes (with separate SVGs for different amounts of detail) and also LTR/RTL variants of a single variant of an icon (say IconMailBold with usage <IconMailBold size={16} />), the naive solution would require the bundle to include all three sizes and their LTR/RTL variants, for a total of 6 SVGs. This is a lot of overhead when a given user (assuming they don't change their locale) will only ever see one of those SVGs. We'd need to figure out a clever way to pre-load the LTR or RTL iconset or dynamic-load the SVGs themselves, but I don't think it's a super straightforward solution.

I bring this up not to get into implementation design, but because it could affect the API - to significantly reduce this overhead, the icon name would also include the size (<IconMailBold16 /> or <IconMailBoldMd />), but that's honestly a bit nasty. I think there might be a nicer automated bundle splitting solution, but it requires some doing.

amcgee commented 3 years ago

See also https://fontawesome.com/how-to-use/on-the-web/using-with/react and https://fontawesome.com/how-to-use/javascript-api/other/tree-shaking for something possibly-relevant

ismay commented 3 years ago

I agree with all these requirements, it's a nice list. A few minor additions:

1. To the last point - developers should also be able to find "base" icons and their available variants, rather than needing to search through a list of all icons with all their variants.  A decent example (although already a bit too complex imho) for reference about discoverability of in size, color, and variant is [FontAwesome](https://fontawesome.com/icons/angry?style=solid)

Yeah maybe we should create a separate issue for documentation. It's important, but feels a bit out of scope for an issue about api requirements. I'll create one.

2. An additional requirement - **The Icons API should facilitate the exclusion of unused icons from the final application bundle, and dynamic loading of minimal icon sets should (possibly) be supported**.  For me this is the main argument for including variants in the component name, rather than as a prop - tree-shaking just works.  We lose that automatic tree-shaking ability for prop-based variation (like size), so we also need to consider the impact on bundle size of these API decisions.

Yeah I agree. Though maybe this is already moving a bit towards discussing the build. It's a bit of both I guess. I'll open an issue for discussing the build as well, since I think we've largely settled on our preferred icon component api. 

3. An additional requirement - **invalid icon props which cannot be caught statically at compile-time should FAIL SAFE to the default icon variant** - i.e. if you select a size which doesn't exist, the default size (or closest size?) should be used, rather than including nothing or a broken image tag

I agree. We concluded something similar here: https://github.com/dhis2/ui/issues/286. Either it should fail during the build, or degrade gracefully and emit some kind of error or warning in the console (in the other issue we settled on a prop-type warning).

As such, we shouldn't include RTL or other dynamic variation in the component name, otherwise we'll end up with user.locale.isRTL ? <IconRTL> : <Icon> sprinkled everywhere we use every singe icon. Practically that probably won't happen and so we will end up with either partial or non-existent RTL support even if we have RTL icons in the library.

I hadn't considered rtl support. What would that entail exactly? Would the icon just be mirrored horizontally? I'm not that familiar with it personally.

Expanding on the bundle size challenge - if we have, say, 3 sizes (with separate SVGs for different amounts of detail) and also LTR/RTL variants of a single variant of an icon (say IconMailBold with usage ), the naive solution would require the bundle to include all three sizes and their LTR/RTL variants, for a total of 6 SVGs. This is a lot of overhead when a given user (assuming they don't change their locale) will only ever see one of those SVGs. We'd need to figure out a clever way to pre-load the LTR or RTL iconset or dynamic-load the SVGs themselves, but I don't think it's a super straightforward solution.

This also straddles both the build and api design a bit it seems. Maybe I could explain a bit what kind of tooling we had in mind for the build, and then after that has been communicated we can see how we'd address this?

See also https://fontawesome.com/how-to-use/on-the-web/using-with/react and https://fontawesome.com/how-to-use/javascript-api/other/tree-shaking for something possibly-relevant

Interesting. This is close to the api I had in mind initially, but after discovering svgr, which CRA uses internally as well, I've started favouring an approach that just leverages that.

ismay commented 3 years ago

I've created:

To discuss the documentation requirements and the way we want to distribute/package ui-icons. I hope that that way we can keep the individual issues as focused as possible on a single topic.

I've revised the requirements a bit to incorporate what @amcgee mentioned (though not everything is included yet, see bottom paragraph):

  1. it should be possible to choose the icon (of course)
  2. it should be possible to set the icon color
  3. it should be possible to set the icon size
  4. it should be possible to set variants, filled, bold, etc.
  5. adding new icons, in new variants or sizes, should not produce breaking changes as much as possible, and be as easy as possible, to prevent any slowdowns in releasing new icons. (I've added this to the build requirements in dhis2/notes#160 as well)
  6. icons should degrade gracefully when an incorrect variant or size is chosen and emit a warning for the app dev, or break the app build to clearly communicate to the dev that the usage is incorrect

I've moved this requirement to dhis2/notes#159: "it should be as easy as possible for developers to discover the available icons". And kept the graceful degradation / clear error message part for the api requirements (see above).

I've also moved what Austin mentioned about tree-shakeability to the build requirements issue, and moved the documentation requirements to the docs issue.

@amcgee That only leaves the rtl discussion for this issue I think. Let me know what you think of the above list of api requirements, and what you'd change/like to discuss, if I've missed anything, etc.

amcgee commented 3 years ago

@ismay thanks, looks really good to me! I think my position on RTL is that is should not be one of the available variants in whatever API we choose, at least not in the same way as other compile-time variants like bold and filled. That leaves us free to implement RTL variants which appear automatically when in RTL contexts (if necessary) without running into the challenges I mentioned above. If we think of any other dynamic runtime variants we can address those separately as well, but for now I think just special-casing RTL is enough 👍

HendrikThePendric commented 3 years ago

Great additions @amcgee! I think I have a slightly different opinion about point 6 "graceful degradation", and - as @ismay mentioned - I proposed a solution with default props in dhis2/notes#162. To highlight the difference, I would end up in a situation where, assuming the icon IconHendrik is only available in size 24:

I like this for various reasons:

HendrikThePendric commented 3 years ago

@cooper-joe RTL support and possible complexities reg that have been discussed above. This makes me wonder how relevant RTL variants for components would be / how often we would encounter them.... Quickly scanning the current icon set of 120 svgs, I only see 2-6 that might need it. Could you please provide some more context info about this?

I am asking because if we are only talking about "a few exceptions", then maybe we can treat them as such, instead introducing a lot of complexity to handle RTL automatically for our icon lib.

cooper-joe commented 3 years ago

@HendrikThePendric I would say we are probably talking about "a few RTL exceptions" for the foreseeable future, so I don't think we should prioritize it. In fact, I would say we don't need to include it now, but ideally it should be possible to include as a feature in the future, rather than a breaking change. I do not think we need RTL functionality now.

HendrikThePendric commented 3 years ago

Thanks for letting us know. I think that's very relevant info 👍

ismay commented 3 years ago

I see here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/RTL_Guidelines that mozilla mentions that icons that mention a direction are mirrored (but not any other icons). Plus, I assume that icons that use the latin alphabet are not as relevant to languages that don't use that alphabet.

HendrikThePendric commented 3 years ago
  • We do have icons with latin characters right? Maybe it'd be good regardless of RTL to generalize those, so they don't refer to a specific alphabet (if possible)?

Some candidates that might need RTL support I noticed in the current set:

  • And to prepare for proper RTL, I guess we should be able to have icons that mention directionality to be mirrored, but leave the other ones as is. That does seem like something that needs to be handled in a wrapper of sorts, so I actually think that if we want to handle that properly without breaking changes, we should take that into account for the icon api already. It'd be good to add that to the requirements I think.

I do see how adding support for RTL later on could lead to a big change in the implementation of the component, but I don't immediately see how it would lead to breaking changes. We would just be adding a prop, or adding logic that adds support for RTL right? So that'd just be a new feature, i.e. a minor version bump?

amcgee commented 3 years ago

I don't think we'd need to make any API changes (or at minimum no breaking changes) since it's a dynamic variant we can deduce from the Context (or even css rules). As such, and given that it doesn't affect many icons, I'd vote for leaving it out of the API for the moment. We can add RTL support as exceptions to the implementation of the few icons that need it, or add directionality to the build process without impacting the API

See also https://material.io/design/usability/bidirectionality.html#mirroring-elements

HendrikThePendric commented 3 years ago

I'd like to move on to discussing the icon props API.

From the final requirements, which can be found here https://github.com/dhis2/ui/issues/285#issuecomment-703725586, and the rest of this discussion, I think we can make the following observations:

So we need to decide how we want to implement size and variant. In theory they could both be implemented as a prop or included in the name. That leaves us with the following options:

  1. Both size and variant as a prop, i.e. <Arrow size={24} variant="filled" />. Personally I would not be in favour of this for the following reasons:
    • Each individual icon would also include an SVG for each variant and each size, so that could be like 3 * 4 = 12. Not great for tree shaking and app-bundle size.
    • Since it is unlikely that we will ever get full "variant-coverage" we will end up with an unpredictable/confusing API.
  2. size as a prop and variant in the component name, i.e. <ArrowFilled size={24} />. Personally I like this API quite a lot, but this is mostly for aesthetic/semantic reasons, I don't really like having the size as being part of the icon name. This option would be slightly better for tree-shaking, but would still mean that importing IconA still effectively means importing each individual sized icons.
  3. Both size and variant in the component name, i.e. <ArrowFilled24 />: I think this option probably makes most sense because it would be best optimised for tree shaking: the app simply imports one single icon component, no 🦍 holding this 🍌 . Furthermore this is also the simplest option to implement, making this library more maintenance friendly.

So to summarise, IMO an icon component should only get color as a prop. RTL should be solved via internal logic, and everything else should be included in the name.

Please let me know if you agree or what you would prefer instead. @amcgee @varl @Mohammer5 @ismay @cooper-joe

ismay commented 3 years ago

I agree. Though I would be very curious to see what others are doing. Just to see if there are any other approaches that we might have overlooked.

Mohammer5 commented 3 years ago

I agree with the proposed approach (No. 3). It also ensures that the icons will always have the size the design specs dictate. On the other hand, I imagine there will be use-cases where an icon will need a different size (e. g. in app specific UI elements, like buttons with non-standard sizes, non-standard Transfer options, etc), so we might have to add an escape hatch to change the size anyways.

varl commented 3 years ago

I vote for option 3.

A component and/or app can still bundle their own custom sizes in addition to what we provide so I don't see the immediate need for an escape hatch and vote to keep the API very restricted for the first implementation.