-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add optional component to be rendered with oui/eui error boundary #669
Comments
I can take a stab at this PR if it is determined to be ok/good approach |
I think this should go through a UX pass before implementing cc @KrooshalUX |
I don't think UX should have any concerns since it is an optional component that wouldn't affect any workflows today? But sure @KrooshalUX please leave your thoughts. My initial thought was that adding additional text about how to report the issue/raise a ticket might be good to include even as a non-optional piece - it looks slightly odd just to have a stack trace with no context |
@derek-ho This is a great proposal for enhancing To give @BSFishy comment some more context - any work that results in either 1) a new component being added to the component library or 2) an enhancement that changes the experience for the end user needs to go through a UX collaboration phase to ensure that the approach is generalized enough for the entire product, provides guardrails for builders to implement quality UI, and provides an excellent end user experience. We are currently working to refine our contribution model and provide more explicit guidance on engaging with OUI for changes - apologies for this not yet being clarified quiet yet. In this case, I would be more aligned to providing builders with configurable props vs defining a generic zone for builders to free-form "shim" a component into. I can take a pass at updating the design of the error boundary component with the items you have called out. I agree with your evaluation about the reporting link being a permanent addition - and, for example, and allowing the URL to be configurable (OpenSearch Project github by default, but a builder can pass a URL to a specific feature repo). In regards to refresh option being something a builder can toggle on/off, is there any concern about an always on refresh button? are there scenarios that refreshing the page might cause additional issues for the end user or their data? Additionally, since error boundaries can be present on varying sizes of elements within the application, we may need to consider different treatment options. |
@KrooshalUX thanks for the response. I think the refresh indication would be text, not a separate button. Here's a message from Nathan Bower: Where the Github and community forum texts are links to the github and forum.opensearch.org, respectively. But that brings up a good question - a lot of the times with an error that would trigger the error boundary it is with the data and a refresh wouldn't necessarily help the situation - so maybe we can decide to exclude it from the message - what do you think? |
@KrooshalUX / @BSFishy any update on this? |
@derek-ho I have some early explorations - the challenge is that the error boundary can be applied to incredibly small components that mean text is not always practical to include. I've got some more research to do, but I'll follow up at the end of the week. |
Is your feature request related to a problem? Please describe.
It seems like the current oui/eui error boundary does not allow you to shim in an optional component to be rendered with the error stack (such as a message to refresh the page or report the issue), adding this as an optional parameter would make this component a lot more extensible - or any other suggestion on how to achieve this in my use case?
Describe the solution you'd like
A clear and concise description of what you want to happen.
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: