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

fix: ErrorBoundary small styling changes #669

Merged

Conversation

AkshatJawne
Copy link
Contributor

Closes #642

Clarification:

  • Only added the centered error for widget related errors, as did not want to disturb the default for panel.py(since we still want non-errored panel to have the items aligned with flex-start), as noted here
    // Apply the same defaults as panel.py
  • Thus, the snippet in the original ticket is wrong, and we can test with any errored snippet, like the one below:
from deephaven import ui
@ui.component
def test_component():
    state, set_state = use_state(20)
    return state

testComp = test_component()

Image below of the icon no longer overflowing when it is made very short:
image

@AkshatJawne AkshatJawne requested review from dsmmcken and mofojed July 25, 2024 19:41
@AkshatJawne AkshatJawne self-assigned this Jul 25, 2024
@@ -204,7 +204,9 @@ function ReactPanel({
direction={direction}
justifyContent={justifyContent}
alignContent={alignContent}
alignItems={alignItems}
alignItems={
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems weird, can't you just give 100% width to the ui-widget-error-view isntead?

@AkshatJawne AkshatJawne requested a review from dsmmcken July 26, 2024 17:10
@AkshatJawne
Copy link
Contributor Author

Ready for re-review

<IllustratedMessage UNSAFE_className="ui-widget-error-view">
<IllustratedMessage
UNSAFE_className="ui-widget-error-view"
UNSAFE_style={{ overflow: 'hidden', width: '100%' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the style stuff to a WidgetErrorView.scss, rather than inline.

@AkshatJawne AkshatJawne requested a review from dsmmcken July 26, 2024 21:35
@@ -19,6 +19,7 @@ import {
getErrorShortMessage,
getErrorStack,
} from './WidgetUtils';
import styles from '../styles.scss?inline';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not like this.

  1. Make a file adjacent to this fill WidgetErrorView.scss
  2. Place your style in it.
  3. Do a regular import of it (vite will handle the rest) you don't need to ?inline and then put it in a style tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was told by @mofojed and @mattrunyon to have it like this, they did not want me to create a separate WidgetErrorView.scss, since it isn't really done like that anywhere else in plugins.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do regular css imports in plugins because they're distributed as a single JS file. Vite and other bundlers typically handle css imports by putting style tags in the index.html, but plugins can't do that

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, you shouldn't need to import the styles here. They should already be included in one of the top TS files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I do not import, the styles don't apply @mattrunyon

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm, yeah they are included here -- had to hard refresh my page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do regular css imports in plugins because they're distributed as a single JS file. Vite and other bundlers typically handle css imports by putting style tags in the index.html, but plugins can't do that.

Oh, didn't know that. Anyways, that's cleaner now thanks.

@AkshatJawne AkshatJawne merged commit d2ec9ed into deephaven:main Jul 29, 2024
14 checks passed
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.

ErrorBoundary Styling Issues
4 participants