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

Make ToggleButtons respect passed style kwarg #1714

Merged
merged 3 commits into from
Sep 14, 2017

Conversation

hinnefe2
Copy link
Contributor

@hinnefe2 hinnefe2 commented Sep 13, 2017

This PR is a (work in progress) attempt to solve #1691.

Things that I've tried so far (which haven't worked) this includes:

  • make ToggleButtonStyle inherit from DescriptionStyle in both the .py and .ts definitions.
  • modify the styleProperties in the .ts definition.

@jasongrout
Copy link
Member

jasongrout commented Sep 13, 2017

Ah, you'll also need to modify the styleProperties like the int slider:

...DescriptionStyleModel.styleProperties,

@jasongrout
Copy link
Member

To explain more: the error is here in the Travis log (which you can find by clicking on the "Details" link above, or on the red X:

lerna ERR! execute src/widget_selection.ts(370,7): error TS2417: Class static side 'typeof ToggleButtonsStyleModel' incorrectly extends base class static side 'typeof DescriptionStyleModel'.
lerna ERR! execute   Types of property 'styleProperties' are incompatible.
lerna ERR! execute     Type '{ button_width: { selector: string; attribute: string; default: any; }; }' is not assignable to type '{ description_width: { selector: string; attribute: string; default: any; }; }'.
lerna ERR! execute       Property 'description_width' is missing in type '{ button_width: { selector: string; attribute: string; default: any; }; }'.

The last line indicates that the styleProperties does not properly include a description_width value, which leads to the above suggestion to augment the styleProperties with the DescriptionStyleModel styleProperties.

Isn't typescript great? In normal javascript, we wouldn't have found out about this omission until we hit a probably hard-to-debug bug where the style properties wouldn't actually update. In this case, the typings force us to fix the bug before things even compile.

@jasongrout
Copy link
Member

@hinnefe2 - thanks again for this PR. I should make a release tonight (i.e., in the next 10 hours or so) - please let me know if you have time to look at the above comment, or alternatively I can look into making the change.

@jasongrout
Copy link
Member

jasongrout commented Sep 14, 2017

Also, one more thing: the name/address on the commit is: hinnefe2 <[email protected]>. If you'd like your real name to show up in our changelog and release notes, can you either:

(a) Tell git your name: git config --global user.name "John Doe", then amend your commit to record that new information, OR
(b) add an entry to our .mailmap file which matches git commit information to current names/emails: https://github.com/jupyter-widgets/ipywidgets/blob/master/.mailmap (add a line that looks like:

Correct Name <correct email> hinnefe2 <[email protected]>

I'm happy to help with either of these.

@jasongrout
Copy link
Member

(I could also make the release tomorrow morning, i.e., within 24 hours, if the extra time helps...)

@hinnefe2
Copy link
Contributor Author

Hi @jasongrout - I think this is good to go. On my local machine the description widths for toggle buttons respect the style argument now.

Thanks for helping me through this - java/typescript is a bit outside my wheelhouse but you made it easy to contribute. I'll definitely be back next time I find something that could use fixing.

@jasongrout
Copy link
Member

Awesome, thanks!

@jasongrout
Copy link
Member

This looks great to me too. I checked it and it works as well. Thanks again!

@jasongrout jasongrout merged commit 7324156 into jupyter-widgets:master Sep 14, 2017
@jasongrout jasongrout changed the title WIP Make ToggleButtons respect passed style kwarg Make ToggleButtons respect passed style kwarg Sep 14, 2017
@jasongrout jasongrout added this to the 7.0.1 milestone Sep 14, 2017
@github-actions github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants