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

Replaced the position absolutes with flex instead in Layers view #5422

Merged
merged 5 commits into from Oct 9, 2023

Conversation

mapsmarketing
Copy link
Contributor

Hi there,

I've only just recently come across this amazing software and felt I could contribute. My contribution is small and it mainly tackles the layers panel. I noticed that position: absolute; is used quite a lot to position the elements which I thought was unnecessary since it can be done more cleanly with display: flex; instead.

This is my first time contributing, so please let me know if I've done anything wrong or if further details are required.

Thank you very much

@mapsmarketing mapsmarketing changed the title Replaced the position absolutes with flex instead Replaced the position absolutes with flex instead in Layers view Oct 2, 2023
Copy link
Member

@artf artf left a comment

Choose a reason for hiding this comment

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

Thank you @mapsmarketing, I really appreciate this PR ❤️

Besides a few comments, I'd also ask to check this regression with borders I see here
Screenshot 2023-10-08 at 16 26 06

Comment on lines 106 to 107
overflow: hidden;
white-space: nowrap;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason behind removing these? Those properties are used to apply ellipsis to long layer names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much on your feedback @artf :)

It's dark-on-dark and I'm blind as a bat, so didn't see the border underneath :D Will fix this up and the CSS code for the ellipsis. I'll resubmit once I've got it updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @artf

I've made the requested fixes. Please let me know if you require anything further.

Thank you very much Artur

image

@artf artf merged commit 2e20af7 into GrapesJS:dev Oct 9, 2023
2 checks passed
@artf
Copy link
Member

artf commented Oct 9, 2023

@mapsmarketing I made some other small fixes/changes but overall you did a great job, thank you very much 🙇‍♂️

@mapsmarketing
Copy link
Contributor Author

Glad it was useful and thank you very much @artf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants