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: Do not use default button type. #5512

Merged
merged 1 commit into from
Oct 22, 2018

Conversation

syranez
Copy link
Contributor

@syranez syranez commented Oct 18, 2018

The default button type is "submit" which triggers a form submit. Thus
if videojs is embedded in a form element the button triggers a form
submit.

This is the same problem with same solution as discussed in #2470

The default button type is "submit" which triggers a form submit. Thus
if videojs is embedded in a form element the button triggers a form
submit.

This is the same problem with same solution as discussed in #2470
@welcome
Copy link

welcome bot commented Oct 18, 2018

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Great catch, thank you!

I searched the source and didn't find any other cases where we'd create a button element that did not have this attribute.

@misteroneill misteroneill added the needs: LGTM Needs one or more additional approvals label Oct 18, 2018
@misteroneill misteroneill changed the title Fix: Do not use default button type. fix: Do not use default button type. Oct 18, 2018
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Oops. Thanks!

@gkatsev
Copy link
Member

gkatsev commented Oct 18, 2018

Yeah, our Button component has that by default which is nice

// Necessary since the default button type is "submit"
type: 'button'

@gkatsev gkatsev merged commit dfcfa45 into videojs:master Oct 22, 2018
@welcome
Copy link

welcome bot commented Oct 22, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@syranez syranez deleted the do-not-use-default-button-type branch October 23, 2018 16:36
gkatsev pushed a commit that referenced this pull request Oct 31, 2018
The default button type is "submit" which triggers a form submit. Thus
if videojs is embedded in a form element the button triggers a form
submit.

This is the same problem with same solution as discussed in #2470
@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Dec 24, 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.

3 participants