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(ui5-card): fires headerPress event upon header click #250

Merged
merged 7 commits into from
Mar 26, 2019

Conversation

ilhan007
Copy link
Member

headerPress is now fired on header click or via Enter and Space keys.
Focus and active states are implemented upon those actions.
Fixes: #243

headerPress is now fired on header click or via ENter and Space keys.
Focus and active states are implemented upon those actions.
@ilhan007 ilhan007 changed the title Feat card header press feat(ui5-card): fires headerPress event upon header click Mar 25, 2019
@ilhan007 ilhan007 requested review from MapTo0 and pskelin March 25, 2019 21:02
Copy link
Member

@MapTo0 MapTo0 left a comment

Choose a reason for hiding this comment

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

Overall - looks good, but I am not really sure if we want to have like ALWAYS a clickable header with pointer, etc. Maybe we can think for a better handling if someone want to have just a visual header without a click interaction

@@ -3,6 +3,8 @@ import URI from "@ui5/webcomponents-base/src/types/URI";
import Bootstrap from "@ui5/webcomponents-base/src/Bootstrap";
import ShadowDOM from "@ui5/webcomponents-base/src/compatibility/ShadowDOM";
import { isIconURI } from "@ui5/webcomponents-base/src/IconPool";
import { isSpace, isEnter, } from "@ui5/webcomponents-base/src/events/PseudoEvents";
import Function from "@ui5/webcomponents-base/src/types/Function";
Copy link
Member

Choose a reason for hiding this comment

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

there is a native type function so I think we can skip this?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is, but the metadata validation would not pass for now. As this refactoring is started by Vladi, I will change it to native Function once it's done.

@@ -82,6 +84,26 @@ const metadata = {
type: URI,
defaultValue: null,
},

_headerActive: {
Copy link
Member

Choose a reason for hiding this comment

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

activeHeader?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is for the pressed state, but there could be a public property determining wether the header is active or not (activeHeader as you said) and then this would be renamed to _pressedHeader or similar.

@@ -133,6 +167,42 @@ class Card extends WebComponent {

super.define(...params);
}

headerPress(event) {
const click = event.type === "click";
Copy link
Member

Choose a reason for hiding this comment

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

I am not really sure how stable is this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this approach is arguable, we can discuss it briefly today.

@CLAassistant
Copy link

CLAassistant commented Mar 26, 2019

CLA assistant check
All committers have signed the CLA.

@ilhan007 ilhan007 merged commit 59b80be into master Mar 26, 2019
@ilhan007 ilhan007 deleted the feat-card-header-press branch March 26, 2019 12:41
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.

3 participants