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

Placeholder color as a variable #2662

Merged
merged 4 commits into from
Apr 7, 2020
Merged

Conversation

jcamejo
Copy link
Contributor

@jcamejo jcamejo commented Mar 17, 2020

Hi @artf, i hope you are doing ok during these strange times.

This small improvement adds a CSS variable to make it possible to change the placeholder bar separately. I have removed the border-color property from the gjs-placeholder class because it always gets overwritten by the sorter function and it was causing a bit of confusion to understand where the color was coming from.

I've added the placeholder color as an constructor option in the sorter object just to make it more clear. It would be nice to find a way to synchronize the Sass variables so there's no need to maintain two variables, I'm not sure if this is actually possible without a custom solution.

Let me know what you think.

Best Regards,

Juan.

@artf
Copy link
Member

artf commented Mar 20, 2020

Thanks @jcamejo actually you're right, it would be great to remove the color reference from the JS file because it's absolutely bad and hard to customize.
The reason behind this choice is was to create this effect
Schermata 2020-03-21 alle 00 02 47
which has different border colors values base on the fact if the placeholder is placed vertically or horizontally.
Don't ask me why I didn't think about using a simple class as a flag to manage this state, but as the code is old I guess it was just inexperience.

Would you like to update this PR to make it work only with the CSS variable and remove the reference from the sorter? I'd really appreciate that :)

If you have any doubt about how to achieve it by using only a class, don't hesitate to ask

@jcamejo
Copy link
Contributor Author

jcamejo commented Mar 24, 2020

I would like to improve the pull request, i will take a look at it soon.

Thanks for the tip, i will ask if i need some assistance.

@jcamejo
Copy link
Contributor Author

jcamejo commented Apr 2, 2020

Hi @artf, good day to you.

I have moved the placeholder color and the orientation properties to the CSS files, let me know what you think.

I was playing with moving the placeholder dimensions properties to SCSS variables as well but i don't think is necessary at this point.

@artf
Copy link
Member

artf commented Apr 7, 2020

Great @jcamejo now it makes much more sense :)

@artf artf merged commit af602c3 into GrapesJS:dev Apr 7, 2020
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.

2 participants