Unrelated CSS attributes being overriden

marcosalles commented 2 years ago

The issue here is the react-drawing-board package is overriding certain css styles on my project and I do not think this was an intended behavior.

Detailed descritption

I installed the react-drawing-board package into my project and noticed that a few elements had sudden changes of style, like font-size, font-color and margins.

Looking deeper into the issue I figured the problem was that these elements were having their styles overriden, much like a CSS reset/defaults stylesheet does (commonly used in design frameworks).

I found out there was a style-sheet being inserted by the lib that, if deleted (directly from the HTML inspector), made most of the elements go back to "normal", so I made some mock elements to represent the behavior I noticed and take some screenshots.

\	without RDB installed	with RDB installed
pics
details	This is the expected behavior. We use a bit of cascading, specially when it comes to font-size and font color. In this example all text is red from the cascaded color, and font-size comes directly from the `<body>` tag	This is after installing the `react-drawing-board` lib. There is some sort of style override that increases font-size, changes colors, adds margins and several other attributes using tag selectors.

Styles used for testing:

I think that these styles are part of the antd lib, a dependency of both this package and an indirect dependency of the father package itself.

I also noticed that the drawing board is not affected by removing this `default` stylesheet.	with default stylesheet	without default stylesheet

dilidili commented 2 years ago

Hi @marcosalles, you are right. The unrelated antd styles have been imported, i will reduce them by directly importing required components style.

For the time being, you can check https://github.com/ant-design/ant-design/blob/3.26.20/components/style/core/base.less to override them.

marcosalles commented 2 years ago

Thank you for you comprehension. This is a good package and I can see it growing in use, so I figured it would be best to report the issue on detail. I didn't try solving this myself because it enters a realm I'm not really familiar with so I could end up messing something up.

Do you have any idea if there might be any styles left-over we should be concerned about when you're done? Do you have any ETA on the solution?

Thanks again for being so helpful =)

dilidili commented 2 years ago

Hi @marcosalles , I've checked the imports and there are no other global side effect styles except for Ant Design. According to the discussion in https://github.com/ant-design/ant-design/issues/9363, I need test some solutions and maybe solve this in a few days.

marcosalles commented 2 years ago

Okay, that's great news, thank you so much!

On Sat, 2 Oct 2021 at 10:01 Wensheng Xu @.***> wrote:

Hi @marcosalles https://github.com/marcosalles , I've checked the imports and there are no other global side effect styles except for Ant Design. According to the discussion in ant-design/ant-design#9363 https://github.com/ant-design/ant-design/issues/9363, I need test some solutions and maybe solve this in a few days.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dilidili/react-drawing-board/issues/24#issuecomment-932748680, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMW2M3QIPEHVA6WFE6ZVF3UE37DRANCNFSM5FDGDZWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

marcosalles commented 2 years ago

Hey there @dilidili, just catching up on the issue, did any of the solutions you test work out to solve this? Is there anything you'd like to point out that I could do to fix it myself?

dilidili commented 2 years ago

Hi @marcosalles, would you help to test the new css bundle in version 0.5.0-beta.2? I have tried some solutions and remove the global style by postinsall scripts.

marcosalles commented 2 years ago

I'll check it out and get back to you, ty!

marcosalles commented 2 years ago

Apparently it can't find the scripts directory:

[5/5] 🔨  Building fresh packages...
[9/19] ⠐ @scarf/scarf
[-/19] ⠐ waiting...
[3/19] ⠐ fsevents
[-/19] ⠐ waiting...
error /path/to/node_modules/react-drawing-board: Command failed.
Exit code: 1
Command: node scripts/postinstall.js
Arguments: 
Directory: /path/to/node_modules/react-drawing-board
Output:
internal/modules/cjs/loader.js:818
  throw err;
  ^

Error: Cannot find module '/path/to/node_modules/react-drawing-board/scripts/postinstall.js'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:815:15)
    at Function.Module._load (internal/modules/cjs/loader.js:667:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47 {

make: *** [install] Error 1

dilidili commented 2 years ago

Apparently it can't find the scripts directory:

[5/5] 🔨  Building fresh packages...
[9/19] ⠐ @scarf/scarf
[-/19] ⠐ waiting...
[3/19] ⠐ fsevents
[-/19] ⠐ waiting...
error /path/to/node_modules/react-drawing-board: Command failed.
Exit code: 1
Command: node scripts/postinstall.js
Arguments: 
Directory: /path/to/node_modules/react-drawing-board
Output:
internal/modules/cjs/loader.js:818
  throw err;
  ^

Error: Cannot find module '/path/to/node_modules/react-drawing-board/scripts/postinstall.js'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:815:15)
    at Function.Module._load (internal/modules/cjs/loader.js:667:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47 {

make: *** [install] Error 1

sry for ignoring scripts dir, fixed in 0.5.0-beta.4

marcosalles commented 2 years ago

I'll get into it right away! Ty.

marcosalles commented 2 years ago

I got an error because although the antd lib is local to react-drawing-board, it's being imported into the father node_modules directory, the one in the main project. I checked and no other lib imports antd.

The script is looking for antd only in the local node_modules directory and therefore throwing an error.

[ERR] Error removing Antd Global Styles: Error: No files match the pattern:
  /path/to/father/project/node_modules/react-drawing-board/node_modules/antd/lib/style/core/index.less

marcosalles commented 2 years ago

I locally linked the library, removed the postinstall step and added the same one to my father project, "solving" the issue.

But that's not the optimal resolution. It would be best if the post install script considered both scenarios.

marcosalles commented 2 years ago

The problem i still there though, even if I manage to build the project. Finally found my way into a working build and noticed nothing changed, the global styles are still there even with the workaround in package.json:

"scripts": {
  "postinstall": "node node_modules/react-drawing-board/scripts/postinstall.js"
},

dilidili commented 2 years ago

The problem i still there though, even if I manage to build the project. Finally found my way into a working build and noticed nothing changed, the global styles are still there even with the workaround in package.json:
"scripts": {
  "postinstall": "node node_modules/react-drawing-board/scripts/postinstall.js"
},

Actually I remove the global styles by the postinstall scripts, so we can not remove the postinsall. you can check the modification here https://github.com/dilidili/react-drawing-board/blob/feat/background/scripts/postinstall.js.

I thought the error is throws because I can not find the antd module path after you install modules in your env, `${process.cwd()}/node_modules/antd/lib/style/core/index.less,`

could you help to provide the pwd of install command runs and the final antd path in node_modules?

marcosalles commented 2 years ago

Sorry it took me long to answer, but what I did was not remove the postinstall script, but move it to my own package.json file with the fixed path.

The actual error it throws is this:

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! react-drawing-board@0.5.0-beta.4 postinstall: `node scripts/postinstall.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the react-drawing-board@0.5.0-beta.4 postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     ${HOME}/npm/_logs/2021-10-20T15_49_43_544Z-debug.log

Here go the directories requested:

# Project dir:
${HOME}/dev/learner

# Final `react-drawing-board` dir:
${HOME}/dev/learner/node_modules/react-drawing-board

# Final `antd` dir:
${HOME}/dev/learner/node_modules/antd

dilidili commented 2 years ago

@marcosalles it is weird, I created a repo with node_modules dir same as you provided above, after I run the npm install from ${HOME}/dev/learner/ and it worked.

which dir you run your npm install? or a reproducible repo would be helpful.

dilidili / react-drawing-board

Unrelated CSS attributes being overriden #24

Detailed descritption