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

GridBox widget #1942

Closed
jasongrout opened this issue Jan 30, 2018 · 21 comments
Closed

GridBox widget #1942

jasongrout opened this issue Jan 30, 2018 · 21 comments
Milestone

Comments

@jasongrout
Copy link
Member

@jasongrout jasongrout commented Jan 30, 2018

Now that CSS grid is making its way into browsers, I think it would be great to have (in core widgets) a GridBox widget that places its widgets in a grid using the CSS grid spec.

@jasongrout
Copy link
Member Author

@jasongrout jasongrout commented Feb 3, 2018

Here are some interesting resources from a HN article on CSS grids: https://news.ycombinator.com/item?id=16292094

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Mar 30, 2018

I'd like to give this a shot.

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Apr 16, 2018

Hi, I had a question -
Should GridBox inherit from Box?

There is a lot of shared code that I could reuse. But Box was meant to be a flexbox implementation. I could (1) create an abstract class, say "CSSLayout" (name TBD, but users shouldn't touch this) and move the common functionality into that. Box and GridBox would both subclass this and set display to either grid or flex.

(Note - I was able to get this to work even otherwise though, just by making GridBox inherit from Box and changing display in the child's constructor )

@jasongrout
Copy link
Member Author

@jasongrout jasongrout commented Apr 16, 2018

I think Gridbox should not inherit from Box. The big thing that Box gives you is a children attribute, but that's probably not useful in a 2d layout to have a single list of children.

@jasongrout
Copy link
Member Author

@jasongrout jasongrout commented Apr 16, 2018

I suppose you could make a CSSLayout widget, but maybe that's not worth it? In my mind, the GridBox was inheriting directly from DOMWidget - but you are in the code, so you have better information than I do.

@jasongrout
Copy link
Member Author

@jasongrout jasongrout commented Apr 16, 2018

Thanks for working on this!!!

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Apr 19, 2018

I agree that the BoxModel contains just a children attribute, but the BoxView seems to contain some logic that we might end up re-implementing ?

that's probably not useful in a 2d layout to have a single list of children.

Seems like a good time to finalize the API - did you have a 2D list (list of lists) in mind, as the input ?
I thought it would be a simple list, and since we will expose the grid spec as part of the Layout, the user can set the grid-template-columns css property to tweak the layout. I've exposed grid, grid_area and grid_gap - following the convention of using only shorthand css properties.

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Apr 19, 2018

Example :

GridBox([w1, w2, w3, w4, w5, w6], layout=Layout(grid='none / repeat(3, 1fr)'))

This will create 2 rows of 3 columns each

@jasongrout
Copy link
Member Author

@jasongrout jasongrout commented Apr 19, 2018

Seems like a good time to finalize the API

Good question. I haven't read too deeply into the grid spec, so I'm not up to speed on what would be a natural api.

Layouts are used as a generic mechanism across all widgets, though. I imagine we would have more specific grid-type properties on the widget itself?

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Apr 19, 2018

I'm not sure what you mean by grid-specific. Involve the grid properties in the constructor or expose them as attributes ?

Some other API suggestions off the top of my head -

  1. This is more like syntactic sugar over Layout
GridBox([w1, w2, w3], {"grid_template_columns": "20% 40% 40%"})
  1. The user gives us a 2D list and we "guess" how it would be laid out ?
GridBox([[w1, w2,  w3], [w3, w4]])  
  1. GridBox will be completely flexible (I do agree that using Layout often is slightly inconvenient). We will build widgets on top of GridBox (l bet VBox and HBox are used more than Grid).

What would you expect (as a user) from GridBox ? If you don't recollect the CSS Grid spec, your idea of a natural API might be more intuitive :)

@maartenbreddels
Copy link
Member

@maartenbreddels maartenbreddels commented Apr 19, 2018

What about looking at the wxWidgets Qt API's. I found the wxWidgets sizers to be quite intuitive. Maybe setting it all at once at the contructor is not the way to go, and the details of how the information layout is encoded should be hidden from users.

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Apr 20, 2018

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Apr 20, 2018

I agree with @Madhu94 about taking a single list of widgets and transparently exposing the CSS grid attributes.

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Apr 20, 2018

I would go with an inheritance of GridBox from Box for children lifetime management, and add the following attributes

  • grid_layout: just like the Layout widget, but of type GridLayout, which would have attributes mapping grid layout attributes

  • grid_items_layout: a list of GridItemLayout that apply to the items. The names of the attributes could be mapped from the CSS ones.

    grid-template-columns -> template_column
    row_start -> grid-row-start
    row_end -> grid-row-end
    column_start -> grid-column-start
    column_end -> grid-column-start
    

@martinRenou
Copy link
Member

@martinRenou martinRenou commented Apr 23, 2018

Hi @Madhu94, I was thinking about working on it today. Did you start already or can I do it?

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Apr 23, 2018

@martinRenou
Copy link
Member

@martinRenou martinRenou commented Apr 23, 2018

No it's ok go on :) I don't want to interrupt you in your work

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented Apr 23, 2018

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Apr 25, 2018

@Madhu94 don't hesitate to show what you have so far if you are working on this.

@Madhu94
Copy link
Contributor

@Madhu94 Madhu94 commented May 9, 2018

#2064 I made a WIP PR, let me know if there is something wrong

@Madhu94 Madhu94 mentioned this issue Jun 28, 2018
@jasongrout jasongrout removed this from the Minor release milestone Jul 6, 2018
@jasongrout jasongrout added this to the 7.3 milestone Jul 6, 2018
@jasongrout
Copy link
Member Author

@jasongrout jasongrout commented Jul 6, 2018

Looks like this is fixed by #2107

@jasongrout jasongrout closed this Jul 6, 2018
@lock lock bot added the resolved-locked label Jun 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants