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

Question/Bug/Suggestion: Unintended CSS Changes #1609

Closed
gunslingor opened this issue Aug 26, 2020 · 15 comments
Closed

Question/Bug/Suggestion: Unintended CSS Changes #1609

gunslingor opened this issue Aug 26, 2020 · 15 comments

Comments

@gunslingor
Copy link

gunslingor commented Aug 26, 2020

So, when I include metro CSS it messes up the CSS associated with GrapesJS, regardless of order. Seems like a common problem I really don't know how to deal with it but I see only a few options:

  • Write my own last/final css to get what's needed; the problem is I think it would be huge and would be just standard css defaults to override what metro is doing globally. It's TONs of work I suspect just to get defaults back for these specific components.
  • I can suggest to the GrapesJS guys that they ensure all defaults that are needed are stated, but I think that would impart the same issue on other packages and people. Only makes the issue worse really. I think grapes and bootstrap are doing it right, I suspect metro is the issues.
  • I can suggest to the Metro guys, what I think is the better approach. Metro is a CSS/JS library, to be used with any and many other libraries, maybe intended to do identical or nearly identical things; so, IMHO, it must be isolated. This means one thing only I think, one change to metro, the CSS shouldn't use anything except custom class selectors (ideally prefixed with "met-" or something to ensure collision doesn't happen).

I reserve the right to be totally wrong, this is just what my eyes are seeing and I'm looking for a solution.

EDIT: I have suspicion I might be able to not use metro-all.min.css, maybe to use all others except common but I couldn't find any real docs on these choice differences.

@gunslingor
Copy link
Author

gunslingor commented Aug 26, 2020

Yeah, this exhibits the same problem. Common is overriding page defaults, 6 other css packages seem to be affected... like metro wants to be the only framework in use.

link("/libs/metro/css/metro-common.min.css", "stylesheet")
link("/libs/metro/css/metro-components.min.css", "stylesheet")

Guess I can't use common, will components work without it? Would hate to have to use this and bootstrap, just to get the common type alignments and such back. Nope, Components doesn't work without common, and no other packages of mine work with common included. Man... that's really disappointing. Strongly recommend fixing this... metro shouldn't be styling ALL of anything except class names in my very humble but very strong opinion.

Let me know if there is a workaround. Desperate, wasted a day trying to fix this. Is metro intended to be used alone as a complete framework, like Django or something? I don't understand why it's negatively affecting literally everything else, even bootstrap and a custom gradient picker are broken... just by including metro css common stuff.

@olton olton closed this as completed Aug 26, 2020
@gunslingor
Copy link
Author

Looks like all those tag selectors in common might be part of a CSS reset not core common components? Still don't understand why it would be in the middle of metro common, shouldn't it be at the beginning or even better separated? And shouldn't this package be able to work without it? What happens when all the CSS vendors start including there own reset versions, it literally means people are going to have to rewrite tons of intended structures to use this.

Not trying to criticize, trying to understand or suggest. No worries, I get the hint, my problem to solve... personally I love free input into my code, couldn't be more appreciative of every word so long as the intent is constructive...

@olton
Copy link
Owner

olton commented Aug 26, 2020

The project is primarily intended for the needs of the company I work for and meets our internal needs. Perhaps, when the project is financed in some other way, the changes you are talking about will be made. Respectfully yours, Sergei Pimenov.

@gunslingor
Copy link
Author

No worries, is financing an option? If I win this contract we can talk if you want. Take care.

@olton
Copy link
Owner

olton commented Aug 26, 2020

Ok, thank you. I collect all comments and suggestions, and in the end most of them find their own implementation.

@olton
Copy link
Owner

olton commented Aug 26, 2020

I collect all comments and suggestions, and in the end most of them find their own implementation. If I closed the issue, this does not mean that I have not taken note of the information and it will disappear. I'm not missing anything, especially reviews and wishes from Metro users.

@olton olton reopened this Sep 12, 2020
olton added a commit that referenced this issue Sep 12, 2020
@olton
Copy link
Owner

olton commented Sep 12, 2020

@gunslingor now in 4.4.0, you can use components without common styles from metro-common.css, metro-reset.css.

@olton olton added this to the 4.4.0 milestone Sep 12, 2020
@gunslingor
Copy link
Author

Beautiful, thank you!

@olton
Copy link
Owner

olton commented Sep 12, 2020

@gunslingor It would be nice if you could test 4.4.0

@gunslingor
Copy link
Author

Sorry for late response, I am currently in the process of moving. How can I download 4.4.0 to test? I search and only found 4.3 versions.

@olton
Copy link
Owner

olton commented Sep 15, 2020

@gunslingor
Copy link
Author

Thanks Olton;

I was able to replace metro-all.min with just these four, which is where I failed on this ticket before due to a css reset if I recall correctly; these seems to work fine now.

        link("/web/static/libs/metro/css/metro-common.min.css", "stylesheet")
        link("/web/static/libs/metro/css/metro-components.min.css", "stylesheet")
        link("/web/static/libs/metro/css/metro-icons.min.css", "stylesheet")
        link("/web/static/libs/metro/css/metro-colors.min.css", "stylesheet")

I think the reset might have been nested inside common which was causing the original problem if I recall correctly, and I think you separated the reset to fix it?

Note that to fully test, I would need to undo a bunch of that css reset-resetting I had to do to make metro-all work with grapejs (due to the presence of the reset); this would be difficult ATM, but as long as all reset code is isolated in metro-reset and metro itself works reasonably well in all browsers with and without it, this problem should be fully addressed and resolved IMHO.

Thanks

@olton
Copy link
Owner

olton commented Sep 16, 2020

link("/web/static/libs/metro/css/metro-reset.min.css", "stylesheet")
link("/web/static/libs/metro/css/metro-common.min.css", "stylesheet")
link("/web/static/libs/metro/css/metro-components.min.css", "stylesheet")
link("/web/static/libs/metro/css/metro-icons.min.css", "stylesheet")
link("/web/static/libs/metro/css/metro-colors.min.css", "stylesheet")

@olton
Copy link
Owner

olton commented Sep 16, 2020

I've separated reset so that you can use your own set to reset styles.

@gunslingor
Copy link
Author

Yeah, or leave it out completely. I tend to never use resets and when I do I stick to the stand normalize.css, to be honest I don't think I've seen one in many years, and can't say I ever recall seeing one that ships with a library like this. The main issue I think is that many packages out there are designed to rely on browser defaults, so if you start changing them you run into the issues I experienced.

bootstrap apparently also ships with a reset, I didn't know that. Seems very odd to me, if someone uses two libraries that each have their own resets... conceptually by themselves the idea of a reset is great, but in practice I find everything still needs to be tested in full in all browsers regardless anyway and so it creates more problems than it solves.

Just thoughts.

@olton olton closed this as completed Sep 28, 2020
olton added a commit that referenced this issue Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants