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 #727 #731

Merged
merged 14 commits into from Nov 2, 2019
Merged

Fix #727 #731

merged 14 commits into from Nov 2, 2019

Conversation

jshjohnson
Copy link
Collaborator

@jshjohnson jshjohnson commented Nov 1, 2019

Description

Fixes the issue detailed in #727.

This removes the ability to pass a placeholder via config to select elements - the implementation was confusing and semantically it is better to pass an option element. It also removes placeholder choices from the choices list for select multiple elements.

Screenshots (if appropriate)

Types of changes

  • Chore (tooling change or documentation change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@codecov
Copy link

codecov bot commented Nov 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@939a73b). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #731   +/-   ##
=========================================
  Coverage          ?   66.18%           
=========================================
  Files             ?       23           
  Lines             ?     1307           
  Branches          ?        0           
=========================================
  Hits              ?      865           
  Misses            ?      442           
  Partials          ?        0
Impacted Files Coverage Δ
src/scripts/templates.js 74.02% <ø> (ø)
src/scripts/choices.js 54.36% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 939a73b...8514ced. Read the comment docs.

@jshjohnson jshjohnson added the bugfix Pull request that fixes an existing bug label Nov 2, 2019
@cypress
Copy link

cypress bot commented Nov 2, 2019



Test summary

116 0 0 0


Run details

Project Choices
Status Passed
Commit c0ad882
Started Nov 2, 2019 12:23 PM
Ended Nov 2, 2019 12:24 PM
Duration 01:20 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 73

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@jshjohnson jshjohnson added breaking change Pull request that introduces a breaking change and removed bugfix Pull request that fixes an existing bug labels Nov 2, 2019
@tinovyatkin
Copy link
Contributor

@jshjohnson IE is green 🎉
download the puppeteer artifact, inspect and replace it as part of this PR

@jshjohnson
Copy link
Collaborator Author

@jshjohnson IE is green 🎉
download the puppeteer artifact, inspect and replace it as part of this PR

🎉 if you can think of a better solution to 59840d1 let me know :D i've run out of free cypress recordings for the month

@tinovyatkin
Copy link
Contributor

@jshjohnson IE is green 🎉
download the puppeteer artifact, inspect and replace it as part of this PR

🎉 if you can think of a better solution to 59840d1 let me know :D i've run out of free cypress recordings for the month

Sure: did you apply to https://www.cypress.io/oss-plan/ ?

@jshjohnson jshjohnson merged commit a0fe05f into master Nov 2, 2019
@jshjohnson jshjohnson deleted the resolve-bug-727 branch November 2, 2019 13:49
@tinovyatkin
Copy link
Contributor

@jshjohnson So, we are gonna have next major soon? I have a couple of breaking changes in mind...

@jshjohnson
Copy link
Collaborator Author

@jshjohnson So, we are gonna have next major soon? I have a couple of breaking changes in mind...

Unfortunately yes. Feel free!

@jshjohnson
Copy link
Collaborator Author

Sure: did you apply to https://www.cypress.io/oss-plan/ ?

I've applied now 🎈

@Fedik
Copy link

Fedik commented Nov 16, 2019

Sorry guys but the fix made the things even more confusing.

What if I want to be able to select an empty options, in multiple list?

<select multiple>
  <option value="11">Option 1</option>
  <option value="">Option 2</option>
</select>

After the fix <option value="">Option 2</option> will be considered as placeholder that is wrong.

An additionally, when I build the same with AJAX.

@jshjohnson
Copy link
Collaborator Author

What if I want to be able to select an empty options, in multiple list?

Why would you want to do that? That's the equivalent of selecting nothing 🤔

@Fedik
Copy link

Fedik commented Nov 18, 2019

That's the equivalent of selecting nothing

to select a "nothing" ;)

but it not a nothing:

<select multiple>
  <option value="11" selected>Option 1</option>
  <option value="" selected>Option 2</option>
</select>

will give you: ["11", ""] that is a "something"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull request that introduces a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants