katoid / angular-grid-layout

Responsive grid with draggable and resizable items for Angular applications.
https://katoid.github.io/angular-grid-layout
MIT License
462 stars 61 forks source link

feat: show an optional background for the grid #81

Closed arnoudb closed 1 year ago

arnoudb commented 1 year ago

Hi, created a proof of concept for showing the grid lines as a background using css.

Sure needs some cleanup but wanted to show you first. If you checkout the branch you can see in the playground how it works. Its quite configurable and you can pick different colors for lines, column background and row backgrounds.

Playing with transparency gives nice results.

Pls let me know what you think! preview2

preview3

llorenspujol commented 1 year ago

Nice work @arnoudb ! 👌 Really awesome solution by not adding any html tag for the background 🚀

I added a commit to support 'rowHeight' = 'fit'. We could also simplify the solution, since both row height and column width are known and can be passed to the CSS variables, but is optional.

About the things TODO, we need to find a better way to decide when the grid is showed or not. I see 2 options:

1- Show the grid background when is not null, and don't show it when null. 2- Add another input like 'showBackground' with the options: 'always', 'whenDragging' (since it may be interesting only on dragging maybe?) and 'never'.

I may prefer the solution number 1 for simplicity. Said that, If you have in mind other options we can discuss them.

Let me know your thoughts about it. After this decision, we will be able to move forward on merging the full feature.

arnoudb commented 1 year ago

Nice cleanup! Didn't realize you were going to commit on this branch as well though :-). We have now diverging branches as i implemented the 'whenDragging' and did some cleaning too. I'll rebase tomorrow. I updated the playground as well to allow for 'never'|'always'|'whenDragging'

llorenspujol commented 1 year ago

Yes, I can commit directly on this branch 😂. It is useful to help you with some commits, so you don't take all the work.

Okay nice! you can solve the conflicts in this branch whatever you like, since before merging to 'main', you will have to squash all commits into a single 1.

I have some doubts about the api with 'never'|'always'|'whenDragging'. I am not sure if adding another 'Input' only for the use case 'whenDragging' is appropriate, but if you have already done let's see.

Also there is still 1 issue left to solve. This issue is related to the background rendering, see the image below:

image

If the border width is big enough is easy to see that the top and left borders are inside the grid item, and the bottom right borders are outside. I think the border of the rows and columns should go inside... but I am not 100% sure right now, since outside can make sense also. I will think about it.

arnoudb commented 1 year ago

Yes, i understand you can! And of course help is welcome. I'm just used that pull request get comments and that the author fixes them 😄

About my code: I took the approach of not adding another @Input. Its part of the config. I have the next logic:

I'll integrate your changes later today and re-commit. About the lines positions: I think i can come up with something. Basically shift the background to the left and top based on lineWidth. I would like it it to stick to whole pixels though. Lets do that in a separate commit. First i'll merge our branches.

arnoudb commented 1 year ago

Oh, i fixed the grid position in the last commit as well. Hope you like the result.

llorenspujol commented 1 year ago

Thanks for you work @arnoudb ! 👏

About the grid position, I have finally opted to just show the border inside the rows and columns... It may not be the best in all cases, but seems solid and it sticks to whole pixels. I also added a new property gapColor, that I think it may be useful in some cases. I did also minimal changes, but that 2 are the most relevant.

Revise those commits, and if you agree with the additions/changes, for me is ready to merge 🚀 !

Before merging, you should squash all those 6 commits into a single 1, and name it something like: feat(grid): added grid background ...

arnoudb commented 1 year ago

Cool, i like the changes! Really like how this thing is working out. Placing the lines inside and also have a gap color makes much sense! I'll double check everything tonight and then will squash the commits.

llorenspujol commented 1 year ago

@arnoudb Thanks for squashing the commits! Now is ready to merge :)

Before doing it, I want to alert you that seems that your commit user is not linked with your actual github user. So you will not appear as a contributor. Here is a link that may help you fixing it.

BTW If you don't care just tell me and I will merge straight away, but is always nice to have this issue fixed in my opnion. I will wait for your response! Thanks again!

arnoudb commented 1 year ago

ah good one! changed the email and pushed again.