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.

Sign up for GitHub

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

Jump to bottom

Fix Dropdown multi option removed update value. #1970

Merged
merged 9 commits into from
Apr 21, 2022
Merged

Fix Dropdown multi option removed update value. #1970

merged 9 commits into from
Apr 21, 2022

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Mar 14, 2022 •
edited
Loading

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR
  • I have added entry in the CHANGELOG.md

@T4rk1n T4rk1n requested a review from alexcjohnson as a code owner March 14, 2022 20:54
@T4rk1n T4rk1n force-pushed the 1868-dropdown-remove-option branch from 411f349 to 424ca71 Compare March 15, 2022 14:03
@AnnMarieW
Copy link
Collaborator

@T4rk1n I really like the refactoring you did here -- Would you like to incorporate #1958 too?

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Mar 15, 2022

@AnnMarieW

Yes, I thought that test and the array join was kinda weird.

@AnnMarieW AnnMarieW mentioned this pull request Mar 18, 2022
Closed
2 tasks
@T4rk1n T4rk1n force-pushed the 1868-dropdown-remove-option branch from 991ae06 to 381f650 Compare March 23, 2022 19:32
@T4rk1n T4rk1n force-pushed the 1868-dropdown-remove-option branch from 381f650 to 84c0e9a Compare April 4, 2022 18:19
@alexcjohnson alexcjohnson mentioned this pull request Apr 12, 2022
Closed
9 tasks
@T4rk1n T4rk1n force-pushed the 1868-dropdown-remove-option branch 2 times, most recently from 42abf07 to a1a9d7f Compare April 18, 2022 15:09
@T4rk1n T4rk1n force-pushed the 1868-dropdown-remove-option branch from a1a9d7f to 8cc4eac Compare April 20, 2022 18:25
alexcjohnson
alexcjohnson reviewed Apr 20, 2022
View reviewed changes
components/dash-html-components/dash_html_components_base/__init__.py
@@ -33,27 +33,27 @@

_js_dist = [
{
"relative_package_path": 'html/{}.min.js'.format(_this_module),
"relative_package_path": "html/{}.min.js".format(_this_module),
Copy link
Collaborator

Choose a reason for hiding this comment

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

ha, we should include this file in our linting / formatting runs. If we did, the new linter would ask for this to become an f-string ;)

alexcjohnson
alexcjohnson reviewed Apr 20, 2022
View reviewed changes
components/dash-html-components/setup.py
@@ -2,22 +2,22 @@
import json
from setuptools import setup

with open('package.json') as f:
with open("package.json") as f:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can delete components/*/setup.py now, right? Also MANIFEST.in, maybe some others too... same in dash-renderer.

Anyway I won't make you do this now, but I'm reminded of it after seeing this file being edited.

alexcjohnson
alexcjohnson reviewed Apr 20, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
alexcjohnson
alexcjohnson approved these changes Apr 20, 2022
View reviewed changes
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Nice job! And excellent tests 🏆

💃 after considering my comments - I think the only blocking one is about the format.black command, the others I'll leave to your discretion.

@T4rk1n T4rk1n force-pushed the 1868-dropdown-remove-option branch from 2739ea8 to ed5942e Compare April 21, 2022 16:11
@T4rk1n T4rk1n merged commit f6b51a8 into dev Apr 21, 2022
@T4rk1n T4rk1n deleted the 1868-dropdown-remove-option branch April 21, 2022 17:20
@JohnMulligan
Copy link

Hello, sorry to comment on a closed issue, but has this fix been included in a release or is it just in the dev branch for now? I'm seeing this behavior in 2.3.1 and just wanted to check.

@ned2
Copy link
Contributor

ned2 commented May 5, 2022 •
edited
Loading

You can see the fix for this in the Unreleased section of the CHANGELOG.md, so I think that means we should see it in the next Dash release.

Fix bug #1908 Selected options not showing when the value contains a comma.

@JohnMulligan
Copy link

This works now. Thanks!!

@HenrikSpiegel HenrikSpiegel mentioned this pull request May 17, 2022
Open
@sergiykhan sergiykhan mentioned this pull request Jun 18, 2022
Closed
@T4rk1n T4rk1n mentioned this pull request Aug 3, 2022
Closed
@petar-qb petar-qb mentioned this pull request Feb 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnnMarieW AnnMarieW AnnMarieW left review comments

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@T4rk1n @AnnMarieW @JohnMulligan @ned2 @alexcjohnson