Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@zamoore I see these errors when I run |
|
@didoo - Yes, those warnings are intentional. For the other 3, I need to discuss with Jory. Seems like the mapping might be slightly off. There are some icons that aren't present in the |
| const carbonName = mapping[baseName]; | ||
|
|
||
| if (carbonName && !registry[baseName].carbon) { | ||
| const carbonMatch = mapping?.match(/Maps to: (.+)/); |
There was a problem hiding this comment.
[thought] @zamoore @jorytindall I am bit worried that this format may lead to people manually editing the description field
What if we instead make it more machine-readable and make people more wary about editing it? In the end, I don't think consumers will ever use this in any way.
There was a problem hiding this comment.
manually editing the description field
Can you clarify what you mean by this? As in, people from our team manually editing this field? Personally I think that's pretty low risk since we all more or less know what is going on with this library. However, part of the purpose of including something in the description was the make sure that the coverage was 100%, so if we deem this not valuable for consumers then I'm fine stripping it out or including something more machine-oriented.
There was a problem hiding this comment.
Let's say we have a new icon or something, and someone adds manually the description field using Map to: *** or Mapping to: *** instead of Maps to: *** because a typo or because they forgot what is the right™ format to use, the script would miss that entry. if it was something more machine oriented like [carbon:***] maybe it would be more obvious that it's meant for a machine, is not just human-readable text.
There was a problem hiding this comment.
Yeah I think updating to something like that makes sense at this point in the process 👍
There was a problem hiding this comment.
I discussed with @jorytindall and we opted for [carbon:icon-name] when available, and ``[carbon:]` when not
packages/flight-icons/scripts/build-parts/generateBundleSymbolJS.ts
Outdated
Show resolved
Hide resolved
packages/flight-icons/scripts/build-parts/generateBundleSymbolJS.ts
Outdated
Show resolved
Hide resolved
zamoore
force-pushed
the
zamoore/hds-6112/flight-icons-figma-file-ssot
branch
from
0be82d9 to
18702e6
Compare
zamoore
force-pushed
the
zamoore/hds-6112/flight-icons-figma-file-ssot
branch
from
18702e6 to
c088dd2
Compare
jorytindall
left a comment
jorytindall
left a comment
There was a problem hiding this comment.
I'm a bit out of my depth here, but cursory examination looks good to me. Left a small point to discuss on the generated catalog, but it's not a blocker, I'm fine either way.
| @@ -10,7 +10,7 @@ | |||
| "fileName": "loading-24", | |||
| "iconName": "loading", | |||
| "description": "loading, loader, spinner, animated", | |||
| "mapping": "No Carbon equivalent.\nTBD", | |||
| "mapping": "", | |||
There was a problem hiding this comment.
I'm not the one to make the decision on this, but just confirming that it's better/easier to have an empty string compared to being explicit with a fallback for icons that aren't mapped. I don't have a strong opinion, but if this field is empty in the catalog should it be excluded?
4 participants
📌 Summary
If merged, this PR will change the source of the flight to carbon icon mapping from the
hds-carbon-icon-map.jsonfile (now deleted) to a single source of truth, thecatalog.jsonfile.Also updates incorrect icons to correct ones (icons added and deleted).
🔗 External links
Jira ticket: HDS-6112