-
Notifications
You must be signed in to change notification settings - Fork 1
COR-526: Popup Button #22
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
Conversation
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.
Change
%button.content_item_button.text-center.popup--open
to:
.content_item_button.text-center.popup--open
that way you wont need to prevent default and it will wont fire any native browser events.
@MKwenhua @ElliottAYoung That suggestion defies the use of semantic elements, however - this is a button and should remain a button, IMO. We shouldn't need to |
@ElliottAYoung Remember that the PR naming conventions specify a capitalized title. This is nitpicky, so historically I've gone through and corrected your PRs, but since it's been a recurring thing, I wanted to mention it - esp. if Cortex is going to be open source, we should present a unified standard that all contributors will be held to |
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.
On second thought I agree with @toastercup , no need to change the button tag
@ElliottAYoung FYI for the future, remember to bump the version once you've wrapped up a feature, otherwise it breaks Cortex develop. As maintainers of this lib, we can bump version within a PR, but contributors do not bump version. |
@toastercup @arelia @MKwenhua
Adds the empty class for ContentItemFieldTypes (only empty for now) and allows for this button to link to the necessary classes to trigger the media popup in Cortex.