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

Grid layout #2107

Merged
merged 2 commits into from
Jul 5, 2018
Merged

Grid layout #2107

merged 2 commits into from
Jul 5, 2018

Conversation

SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Jun 28, 2018

screen shot 2018-07-05 at 12 18 18 am

@SylvainCorlay SylvainCorlay changed the title [WIP] Run tests Grid layout Jun 28, 2018
@Madhu94
Copy link
Contributor

Madhu94 commented Jun 28, 2018

Both commits taken together, my changes are effectively wiped out and might as well not be included ! :(
The changes bear close resemblance to what the state was when I made this comment #1942 (comment) => Basically GridBox inherited from Box, and all CSS properties (not redundant) were in Layout.

Could you please let me know what were the reasons for proposing it should be a separate class ?
Thanks.

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Jun 28, 2018

Both commits taken together, my changes are effectively wiped out and might as well not be included ! :(
The changes bear close resemblance to what the state was when I made this comment #1942 (comment) => Basically GridBox inherited from Box, and all CSS properties (not redundant) were in Layout.

Yeah, I am really sorry if you spent too much time on separating it in multiple widgets.

So I was playing with your implementation yesterday and my initial change was to group things using the shorthand properties like elsewhere in ipywidgets. Although I did not go all the way, because there is a catch-all grid shorthand property that is pretty hard to deal with from a user's standpoint.

Then we discussed with @jasongrout and went back & forth between the initial design of adding grid properties to layout that I think you proposed and your PR.

In any case, I would be happy to squash my commit into yours especially as you pretty much had this version already earlier.

From what I gather in the original thread, you seem to lean towards a single layout attribute. Also, coming back to your proposal of having a hierarchy of Layout types, it seems that at least the item properties should be in the base one, and maybe only the container grid properties should be in the GridBoxLayout class.

What do you think? Should we go ahead with something like this or would you like to iterate more upon it in the earlier PR?

@Madhu94
Copy link
Contributor

Madhu94 commented Jun 29, 2018

From what I gather in the original thread, you seem to lean towards a single layout attribute. Also, coming back to your proposal of having a hierarchy of Layout types, it seems that at least the item properties should be in the base one, and maybe only the container grid properties should be in the GridBoxLayout class.

The current state is similar to how Box is implemented though.

Should we go ahead with something like this or would you like to iterate more upon it in the earlier PR?

No, please go ahead with your changes, my apologies if the earlier comment seemed a little aggressive.

@SylvainCorlay
Copy link
Member Author

Should we go ahead with something like this or would you like to iterate more upon it in the earlier PR?

No, please go ahead with your changes, my apologies if the earlier comment seemed a little aggressive.

Not at all!

@SylvainCorlay SylvainCorlay force-pushed the gridbox branch 4 times, most recently from 8e2759f to c0071fc Compare July 4, 2018 18:20
@SylvainCorlay
Copy link
Member Author

@jasongrout this is ready to go!

@SylvainCorlay SylvainCorlay merged commit 446d608 into jupyter-widgets:master Jul 5, 2018
@SylvainCorlay SylvainCorlay deleted the gridbox branch July 5, 2018 09:25
@jasongrout jasongrout mentioned this pull request Jul 6, 2018
@jasongrout jasongrout added this to the 7.3 milestone Jul 6, 2018
jasongrout added a commit to jasongrout/ipywidgets that referenced this pull request Jul 6, 2018
This was accidentally committed in jupyter-widgets#2107, but shouldn’t be there since we are using the yarn.lock in the repo root.
@lock lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 21, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2020
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.

3 participants