-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(pictograms): update berlin-brandenburg name #4610
fix(pictograms): update berlin-brandenburg name #4610
Conversation
- trash | ||
- upload | ||
- upload--alt | ||
- cloud--download |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have prettier enabled for YML files by any chance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we did since these changes were formatted on save. I only actually changed line 48 and then the other spaces were added automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnm2377 I don't think we have it enabled yet for YML files https://github.com/carbon-design-system/carbon/blob/master/package.json#L21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine, did you want to add in the fallback too, just in case?
Deploy preview for the-carbon-components ready! Built with commit 5a395d0 https://deploy-preview-4610--the-carbon-components.netlify.com |
Deploy preview for carbon-elements ready! Built with commit 5a395d0 |
Deploy preview for carbon-components-react ready! Built with commit 5a395d0 https://deploy-preview-4610--carbon-components-react.netlify.com |
@jnm2377 looking at CI, seems like something is up with the SVG parsing 🤔 would you want to redo the asset to see if that fixes it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, just a quick note: this landmark is called the "Brandenburg Gate". In the files, brandenburg and brandenberg are used interchangeably.
This did not come from this PR but may be a good opportunity to correct it.
packages/pictograms/metadata.yml
- Line 272
- Line 276 (in the comment)
- Line 277
packages/pictograms/svg/berlin--brandenburg--gate.svg
- Line 5
- Line 7
packages/pictograms/svg/berlin--brandenburg--gate.svg
- Line 7
- Line 9
@janhassel I'm not sure what you mean by
The svg files are created by design, I believe, and there aren't any changes to it since the icon is the same. This is just changing the svg file name itself and the metadata. |
…omponents into update-berlin-pictogram
@joshblack any idea why it would say the icon isn't included in the icon source folder, but the svg is there? |
@jnm2377 I am referring to the friendly names in the metadata file and the IDs in the svgs. I just came across this Pull Request because I noticed the typo in the icon name and wanted to raise an issue about that. I thought it might be a good idea to already correct it here instead of creating another Pull Request, but like I said - the typos were not caused by you and hence you should feel no obligation to fix it 🙂 Maybe this would be a question for @joshblack then: Should I better open a separate issue for this? |
When is this going to be deployed? |
@rcwakelin this can be merged once I can figure out why ci tests are failing. Then it would be included in our weekly patch fix next week. |
@jnm2377 just updated! Seems like there was an |
ahhhh duh 🤦♀ thanks! @joshblack |
Closes: https://github.ibm.com/brand/pictograms/issues/2 (private Brand repo)
cc: @mjabbink
cc: @dudley-ibm
Ref: #4590
Changed