ThatOpen / engine_components

MIT License
349 stars 134 forks source link

MainToolbar not showing in React+vite environment #113

Closed MiguelG97 closed 11 months ago

MiguelG97 commented 1 year ago

Describe the bug 📝

Hi folks, I've followed the tutorial path for generating clipping planes image

however, I dont see the toolbar showing up in the viewer: image

Reproduction ▶️

No response

Steps to reproduce 🔢

No response

System Info 💻

openbim-components

Used Package Manager 📦

npm

Error Trace/Logs 📃

No response

Validations ✅

julillosamaral commented 1 year ago

I'm having the same issue! But I think the button is actually in the HTML (check the picture) I think the issue is on the styles not being loaded or the dimension tool having a bug.

When I click that "straighten" button it does nothing

image

julillosamaral commented 1 year ago

@MiguelG97 I think this is not the right thing to do, but for the moment...

If you copy the content from: https://ifcjs.github.io/components/resources/styles.css Into a css file in your project and include in one of the components you're working with the dimensions tool works fine and the toolbar is displayed fine.

I'll continue researching to check if this styles are included locally and how to load them, maybe it's a missing config on the React projects

Alia2006 commented 1 year ago

Your need to settle the css and ico issues first.

tb-viktor commented 1 year ago

We are also experiencing this issue with Vite+React.

If you copy the content from: https://ifcjs.github.io/components/resources/styles.css Into a css file in your project and include in one of the components you're working with the dimensions tool works fine and the toolbar is displayed fine.

Directly importing this file is a solution, but not a good one. The styles inside of the file are generic and not scoped to openbim components. This means that they apply globally and every other element on the page that is rendering the view will also be affected by the values.

Is there a proper fix on the way for this issue?

HoyosJuan commented 12 months ago

Hi there!

Actually, copying the styles.css file in your app is the solution right now if you want to use the built-in UI. Keep in mind you can also create all the UI by yourself, as using the built-in is optional.

We're aware (issue https://github.com/IFCjs/components/issues/135) that some styles may conflict with the ones in the main app, that's because some styles in the file target general HTML elements. We can get rid of those general HTML styles to not affect the existing ones in the app, and only left the classes.

Also, we can set a prefix for all classes to further scope them in the library, but at the time being we don't see it needed due that we're using utility classes. If you have the same class names in a custom CSS (very unlikely if not using a utility CSS library meaning they won't conflict) then yours will replace the existing ones if you set your CSS last.

But, just for your knowledge, we're looking for a way to make the built-in UI more customizable.

HoyosJuan commented 11 months ago

Hey @MiguelG97!

Is ok for you if we conclude this issue here? Or is there anything else going on about this. Let me know please! I'm following up all issues at the time.

NiklasPor commented 11 months ago

There's definitely a need for scoping utility classes. There are a ton of other utility frameworks (PandaCSS, ..) and not all classes mean the same in every app. The style clashing issues aren't always easy to find and I don't think anyone enjoys trying to find out that weird layout bug that started happening, when they added openbim components in their production app.

For a workaround for now, if you're using SCSS or a similar postprocessor, you can simply scope all of the styles to one shared parent container (the editor container would be a good idea):

.editor-container {
  // Paste the styles from https://ifcjs.github.io/components/resources/styles.css here
}

Furthermore I'd heavily suggest that the styles should be served together with the openbim-components package (as an optional resources), as otherwise version changes might start breaking the UI (when more classes are added).

HoyosJuan commented 11 months ago

Hey, this have been solved in PR https://github.com/IFCjs/components/pull/235. Between setting a prefix and scoping to the viewer container, we've decided to scope it to the viewer container for these reasons:

  1. All UIComponents are always located inside the viewer container.
  2. We don't need to update the whole UIComponents with a prefix.
  3. For people using Tailwind in their apps, they can simply build the CSS based on the configuration from the library and that would get rid of the scoping, meaning they can match the viewer styles with theirs.

Please, keep in mind the PR was merged, but the package is not yet updated with a new patch in NPM.

Thanks guys!