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

fix(ui5-panel): add missing dependency for ui5-icon #406

Merged
merged 3 commits into from
May 18, 2019

Conversation

Psidium
Copy link
Member

@Psidium Psidium commented May 17, 2019

When importing the ui5-panel by itself, it would not be able to load the ui5-icon that it makes use of.

Example:

Rendering the panel with

<ui5-panel width="100%" accesible-role="Complementary"></ui5-panel>

Without the fix:
Screen Shot 2019-05-17 at 20 27 14

With this fix, it works as expected:
Screen Shot 2019-05-17 at 20 29 58

Also, I did not find how to test this implementation.

@CLAassistant
Copy link

CLAassistant commented May 17, 2019

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Gabriel Borges seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Hello @Psidium
thanks for your finding and proposal. In addition to the Icon import (you already did), could you also do the following:

replace line 278:
await fetchResourceBundle("@ui5/webcomponents");

with:
await Promise.all([
fetchResourceBundle("@ui5/webcomponents");
Icon.define(),
]);

As the Panel depends on the Icon, it should await for the Icon`s define.

@ilhan007 ilhan007 merged commit 650bcb0 into SAP:master May 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants