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

feat(icons): Update icons.yml 0916 #9595

Merged
merged 22 commits into from
Oct 5, 2021
Merged

Conversation

LMapes
Copy link
Contributor

@LMapes LMapes commented Sep 3, 2021

Updated icons.yml with

aliases
friendly names

approved by BXD

Changelog

Changed

  • icons.yml

changes made correspond to issue #9473

@netlify
Copy link

netlify bot commented Sep 3, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 25fc1c5

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/615c7507d1e23e000781c499

😎 Browse the preview: https://deploy-preview-9595--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Sep 3, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 25fc1c5

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/615c750758ec0e0008ec0220

😎 Browse the preview: https://deploy-preview-9595--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Sep 3, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 25fc1c5

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/615c75075689a20007c89ffc

😎 Browse the preview: https://deploy-preview-9595--carbon-components-react.netlify.app

updated metadata
@LMapes LMapes marked this pull request as ready for review September 16, 2021 15:41
@LMapes LMapes requested a review from a team as a code owner September 16, 2021 15:41
@LMapes LMapes changed the title 0903Update icons.yml feat(icons): Update icons.yml 0916 Sep 16, 2021
@jnm2377
Copy link
Contributor

jnm2377 commented Sep 17, 2021

Looks like some tests are failing for this.

@LMapes
Copy link
Contributor Author

LMapes commented Sep 17, 2021

@jnm2377 I don't know how to fix these errors.

@jnm2377
Copy link
Contributor

jnm2377 commented Sep 17, 2021

Looks like it's breaking bc of bad indentation. I would double check your formatting to make sure there aren't other bad indentations besides this one.
Screen Shot 2021-09-17 at 3 08 15 PM

Also, public tests might need to be updated. I'm not sure. But looking at the PR you linked, it definitely updated test after adding new icon data.
Screen Shot 2021-09-17 at 3 09 41 PM

packages/icons/icons.yml Outdated Show resolved Hide resolved
@LMapes
Copy link
Contributor Author

LMapes commented Sep 17, 2021

@joshblack I don't know how to fix indentation. It looked visually okay to me in VS Code.

packages/icons/icons.yml Outdated Show resolved Hide resolved
packages/icons/icons.yml Outdated Show resolved Hide resolved
packages/icons/icons.yml Outdated Show resolved Hide resolved
packages/icons/icons.yml Outdated Show resolved Hide resolved
packages/icons/icons.yml Outdated Show resolved Hide resolved
packages/icons/icons.yml Outdated Show resolved Hide resolved
packages/icons/icons.yml Outdated Show resolved Hide resolved
@LMapes LMapes requested a review from a team as a code owner September 21, 2021 21:15
@mjabbink
Copy link

mjabbink commented Oct 4, 2021

@LMapes @jnm2377 Did the naming get resolved on this issue? I’m seeing lots of code names showing up in the library that I think this issue will resolve.

@LMapes
Copy link
Contributor Author

LMapes commented Oct 4, 2021

@mjabbink I added all the metadata content for all the UI icons into the .yml. Why it's not displaying by now is unclear to me.

@jnm2377
Copy link
Contributor

jnm2377 commented Oct 4, 2021

Hey @LMapes sorry if I didn't make it clear in the past comments. I brought up tests failing a while back, which need to be addressed before this PR can get merged. Typically, contributors are responsible for updating their PRs accordingly when we give feedback or leave reviews. I tried to help out by updating all of the indentation errors your code had, but looks like the snapshots still need to be updated since you've added new metadata.

packages/icons/icons.yml Outdated Show resolved Hide resolved
@LMapes
Copy link
Contributor Author

LMapes commented Oct 4, 2021

@mjabbink I don't know how to correct this. Please assign a Carbon dev.

@Katie-A-IBM
Copy link

@joshblack @jnm2377 @tay1orjones
Hi All, Would you be able to set up a call to discuss this issue?
Thanks
cc @LMapes

@tay1orjones
Copy link
Member

@LMapes I'm happy to fix it, but don't know what the proper values should be. This is what it is currently:

- name: map--identify
  - map
    - travel
    - point
    - identify
    - ID
    - center
    - locate
    - location
    - graph
  sizes:
    - 32

There's no friendlyName or aliases entries. They were there previously but were removed as part of this PR. Should it be this instead?

- name: map--identify
  friendly_name: map--identify
  aliases:
    - map
    - travel
    - point
    - identify
    - ID
    - center
    - locate
    - location
    - graph
  sizes:
    - 32

@LMapes
Copy link
Contributor Author

LMapes commented Oct 5, 2021

@tay1orjones Thanks for clarifying what you need. This should work

  • name: map--identify
    friendly_name: Map identify
    aliases:
    • map
    • travel
    • point
    • identify
    • target
    • exact
    • ID
    • center
    • locate
    • location
    • graph
      sizes:
    • 32

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

I've corrected the map--identify metadata as well as another piece of indentation I found. There were also a few entries that had the .svg suffix added, which isn't needed and caused the build to fail.

@kodiakhq kodiakhq bot merged commit 61b5aaa into carbon-design-system:main Oct 5, 2021
@mjabbink
Copy link

mjabbink commented Oct 7, 2021

@LMapes I’m not sure if this issue covers all of these naming issues (below list) but these all need attention. The below names were copied directly from the library as they appear. The friendly names are missing for all of the below.

continue
continued--filled
fit-to-height
fit-to-width
settings--services
settings--view
cost
cost--total
document--signed
document--unprotected
document--protected
document--security
result--new
result--old
result--cancelled
rule--draft
rule--filled
rule--test
task--add
task--asset-view
task--approved
task--complete
task--location
task--settings
task--star
task--tools
ungroup-objects
group-objects--new
group-objects--save
window--black-saturation
image--search--alt
airplay--filled
asset--confirm
asset--digital-twin
asset--view
radio--combat
radio--push-to-talk
wifi--controller
phone--application
location--save
map--center
map--identify
dog-walker
user--access
user--military
user--service-desk
user--settings
carbon-accounting
energy--renewable
solar-panel
temperature--celsius
temperature--celsius--alt
temperature--fahrenheit
temperature--fahrenheit--alt
tropical-storm--tracks
wind-power
cell-tower
drone--delivery
drone--front
drone--video
cloud--offline

@LMapes
Copy link
Contributor Author

LMapes commented Oct 7, 2021

@mjabbink All of these were in the metadata yml file for this issue and should be appearing on screen whenever the IDL update happens

@mjabbink
Copy link

mjabbink commented Oct 7, 2021

@LMapes ok great, I was not sure all of these were included. thanks for the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants