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

refactor(ui5-card): header slot is added #3490

Merged
merged 13 commits into from Jul 28, 2021
Merged

refactor(ui5-card): header slot is added #3490

merged 13 commits into from Jul 28, 2021

Conversation

GerganaKremenska
Copy link
Member

BREAKING CHANGE: titleText, subtitleText, status, headerInteractive properties, action and avatar slot and header-click event are move to component, and the card now has a header slot, instead of all the properties.

Copy link
Contributor

@gmkv gmkv left a comment

Choose a reason for hiding this comment

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

Please update all uses of <ui5-card> in the test pages.

packages/main/src/Card.js Show resolved Hide resolved
packages/main/src/Card.hbs Show resolved Hide resolved
@GerganaKremenska
Copy link
Member Author

Please update all uses of <ui5-card> in the test pages.

done

Copy link
Contributor

@gmkv gmkv left a comment

Choose a reason for hiding this comment

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

The card markup needs to be fixed in -
http://localhost:8081/test-resources/pages/Timeline.html
http://localhost:8080/test-resources/pages/Carousel.html
http://localhost:8080/test-resources/pages/Components.html
http://localhost:8080/test-resources/pages/Kitchen.html
http://localhost:8080/test-resources/pages/Kitchen.openui5.html

Add mention of ui5-card-header in docs/Public Module Imports.md and package/main/README.md

All the indentations in the files you've edited have been converted to spaces. So you need to format them back to tabs.

gmkv
gmkv previously approved these changes Jul 7, 2021
gmkv
gmkv previously approved these changes Jul 7, 2021
@gmkv gmkv closed this Jul 8, 2021
@gmkv gmkv reopened this Jul 8, 2021
@gmkv gmkv closed this Jul 9, 2021
@gmkv gmkv reopened this Jul 9, 2021
@gmkv gmkv closed this Jul 12, 2021
@gmkv gmkv reopened this Jul 12, 2021
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.

Looks good, there are some inline comments, that you can find.

@ilhan007 ilhan007 dismissed stale reviews from vladitasev and themself July 23, 2021 20:40

outdated

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.

One thing, that I noticed, not sure if we need to address it.

If a header slot is used with a custom header, not the CardHeader, the separator line between the header and content parts is not displayed (probably it's part of the CardHeader).

Did you already discuss this internally? Should we draw this line, or let the application developers add it to the custom header (which might be not so convenient as they should now the CSS param for the color of the line)? I will accept any decision from your side.

@GerganaKremenska
Copy link
Member Author

One thing, that I noticed, not sure if we need to address it.

If a header slot is used with a custom header, not the CardHeader, the separator line between the header and content parts is not displayed (probably it's part of the CardHeader).

Did you already discuss this internally? Should we draw this line, or let the application developers add it to the custom header (which might be not so convenient as they should now the CSS param for the color of the line)? I will accept any decision from your side.

We have discuss it and we will add(Card component) the separator line.

Copy link
Contributor

@dimovpetar dimovpetar left a comment

Choose a reason for hiding this comment

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

Besides header separator LGTM

@GerganaKremenska GerganaKremenska dismissed dimovpetar’s stale review July 27, 2021 14:22

please retest, the saparator is there

@ilhan007 ilhan007 merged commit 86f0333 into master Jul 28, 2021
@ilhan007 ilhan007 deleted the card_refactoring branch July 28, 2021 05:27
@gmkv
Copy link
Contributor

gmkv commented Jul 28, 2021

Part of #3107

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.

None yet

5 participants