feat: EU component Icon #15
Conversation
|
deploy/netlify failed - the problem is probably on the environment configuration side. Do you not have to configure the webpack.config.js like here file for the EU in the same way as for the EC file to correctly display the svg file? |
| "access": "public" | ||
| }, | ||
| "devDependencies": { | ||
| "@ecl/ec-resources-icons": "^2.0.0" |
There was a problem hiding this comment.
@nimek2 , the thing here is that you are providing a component as part of an ecosystem, in this case is EU so your resources should come from the EU implementation of ECL in this case, and all of your imports then would use the resources provided by this package, not the EC one.. ;)
For the rest yes, the problem about the icons not rendering is due to webpack conf, i will fix that, once you fix this EC-EU confusion the pull request will be merged, we already reviewed the EC version.
Thanks!
There was a problem hiding this comment.
I changed and commited the resource from ec-resources-icons to eu-resources-icons, is it enough?
| import path from 'path'; | ||
| import { renderTwigFile } from '@ecl-twig/test-utils'; | ||
|
|
||
| import brandedIcons from '@ecl/ec-resources-icons/dist/lists/branded.json'; |
There was a problem hiding this comment.
Now that we are fetching the eu package for the resources we should use the icons from that one, so here we would import from the EU package and not the EC one.
| import notificationsIcons from '@ecl/ec-resources-icons/dist/lists/notifications.json'; | ||
| import uiIcons from '@ecl/ec-resources-icons/dist/lists/ui.json'; | ||
| import defaultSprite from '@ecl/ec-resources-icons/dist/sprites/icons.svg'; | ||
| import brandedIcons from '@ecl/eu-resources-icons/dist/lists/branded.json'; |
There was a problem hiding this comment.
I guess there is a confusion between EC and EU here...
EC components (as this file), should use EC dependencies, no EU, and the same for the other way
There was a problem hiding this comment.
Fixed here 7b43a89
That was stupid mistake
| @@ -0,0 +1,40 @@ | |||
| # ECL Twig - EC Icon component | |||
There was a problem hiding this comment.
there should be no reference to EC in this file (including npm package below, and so on)
| import { withNotes } from '@ecl-twig/storybook-addon-notes'; | ||
| import withCode from '@ecl-twig/storybook-addon-code'; | ||
|
|
||
| import brandedIcons from '@ecl/ec-resources-icons/dist/lists/branded.json'; |
There was a problem hiding this comment.
should be EU everywhere here too
|
|
||
| import brandedIcons from '@ecl/ec-resources-icons/dist/lists/branded.json'; | ||
| import generalIcons from '@ecl/ec-resources-icons/dist/lists/general.json'; | ||
| import notificationsIcons from '@ecl/ec-resources-icons/dist/lists/notifications.json'; |
There was a problem hiding this comment.
should be EU everywhere here too
|
All pathes fixed in latest commits. I will pay more attention to the correct paths of resources for next PRs, so that we will not repeat this problem, thanks for your help. |
|
Almost there. |
|
All fixes done (including focusable parameter) |
nimek2
added
pr: review needed
|
Aria-hidden parameter added. SO now we are good here? |
yhuard
removed
the
pr: review needed
yhuard
removed
pr: review needed
yhuard
removed
pr: modification needed
pr: review needed
No description provided.