RedHeadphone / react-json-grid

🔄 Effortlessly transform complex nested JSON into grid tables
https://npmjs.com/package/@redheadphone/react-json-grid
MIT License
18 stars 2 forks source link

Table may have unexpected bottom margin with an improper global css configuration. #24

Closed cainmagi closed 1 week ago

cainmagi commented 1 week ago

Description

In some cases, the site toolkit may contain a global css file with a configuration like this:

table {
    margin-bottom: 2em;
}

This configuration will be applied to all tables, including the tables created by react-json-grid:

image

In the above figure, the top-level table has an unexpected background stretching out from the table. That's because the global margin-bottom takes effects on it.

Expect

For the following codes

https://github.com/RedHeadphone/react-json-grid/blob/7f6f1564f33e58997bedeb14f62439e1da5c3d7e/src/styles.scss#L27-L48

I think it is better to make the top-level table.json-grid-table {...} also configured with a margin: ....

Another choice is to expose the className and style as a customizable property of <JSONGrid/>. That would be helpful for users to perform some specific customizations on the styles.

Current version

RedHeadphone commented 1 week ago

handling global CSS should be part of the developer using the library, adding margin in library table to avoid global margin seems incorrect.

You can fix the example shown in the image by adding JSONGrid component in a span tag with class json-viewer-container and adding CSS as below

.json-viewer-container table {
  margin-bottom: 0;
}

Based on your second choice, what we can do is have deterministic class name. So the class which is currently styles_json-grid-table__VFxOm will become json-grid-table. This change would only require rollup config change I think.

cainmagi commented 1 week ago

I agree. Actually, I managed it by another way:

<div className="container">
   <JSONGrid/>
</div>

and then define a global scss class

.container {
  & > div > table {
    margin-bottom: 0;
  }
}

But I still think it is useful to make className customizable. Maybe you can consider to make NestedJSONGrid exported in the package and allow users to import the component like this

import JSONGrid, { NestedJSONGrid } from "@redheadphone/react-json-grid";

It will be more helpful for others to make some deeper customization. At least, if it is possible to add this feature, it will be very helpful to me (I am developing a dash version of this useful package now).

RedHeadphone commented 1 week ago

Ok so part of this issue will do following 2 things

Both changes doesn't affect main usecase so it should be okay.

cainmagi commented 1 week ago

Thank you! I am looking forward to seeing the next version.

RedHeadphone commented 1 week ago

I’ve completed the first task—configuring the Rollup setup for deterministic class names (like json-grid-table instead of hashed ones).

However, second task doesn't seem correct. Without exporting all the functions that are used in the main container component, the NestedJSONGrid component alone becomes useless and exporting everything isn’t ideal.

Based on your use-case, you should be able to achieve what you need with just the first change. If that doesn't work out, you can just fork this repo.

RedHeadphone commented 1 week ago

@cainmagi let me know if I can close this issue!

cainmagi commented 1 week ago

Thank you! That's good enough to me. I can close it by myself.

RedHeadphone commented 1 week ago

@cainmagi Both changes are now published with version 0.9.1 Thanks again for reporting the issues, and let me know if you find any more issues!