-
Notifications
You must be signed in to change notification settings - Fork 4
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
Runtime Discovery Editor #551
Conversation
nerik
commented
Jun 5, 2023
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
26ec046
to
20cedd3
Compare
This is the state of things: Untitled.movI'd be interested in hearing your thoughts about the approach @danielfdsilva @hanbyul-here ... Now to tackle error handling |
It is amazing to see how you approached this problem. Such an exciting prototype with a relatively small amount of code 👯 A few thoughts after playing with it. (Maybe too detailed at this point?) I tried to make errors in various ways. I have found two ways to make errors so far. 1. MDX syntax level (ex. A draggable code editor is a brilliant idea to work around some problems quickly. But while trying to edit, I realized that dragging is also needed for editing (duh) 🤔 maybe some mechanisms to unlock/lock the code editors dragging? How do you picture the output of this editing process to be saved? Local files? GitHub? |
…#556) This is a fix for an overdue bug. (that can help #551) We want to suppress cra overlay for errors already handled on the component level, and this suppression was not working because the style selector was not correct. (I realized that this is not the main problem for mdx editor. We would need some sort of logic to clear out errors when an error is fixed. but this one is a good one to fix anyway.) I sneaked in a little documentation change and other little code changes. Since we enforce users to wrap everything with `<Block>`, Figure doesn't need to be wrapped with error boundary.
@nerik This is quite an interesting approach and it's nice to get the immediate preview! :D I spent some time thinking about the error handling, but it is not straightforward to do with the current block scoped approach. I tried to see if we could compare the props of a given block and clear the error if they were different, but since the mdx get evaluated on every render, the components are always different even if the content did not change, so this approach is a no go. |
@hanbyul-here @danielfdsilva This is ready for review. And merge if we all agree on #559 (comment) This is new since we spoke last time:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such an exciting change 👯
We can consolidate the diverged approaches to achieve a similar goal inside the applications. ex. Analysis page and MDX editor use both local storage but in different ways. + I wonder if we can just use ErrorBlockBoundary
component for block error handling.
But I think it is more important to test this idea fast. so if you want to consider the integration with the existing architecture later, I am down with it.
Interesting, we might even be able to have a pseudo "user parameters" page where a user could see saved AoIs, analysis params, discovery draft, etc... With the obvious caveat that this is tied to the current browser, which must be clearky communicated.
Cool! Let's also see what comes out of #561 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: all files and folders on veda follow the kebab-case syntax. Would be good to maintain consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CRA overlay is still popping up. I'd suggest we change the selector to the following one so it is more generic. It is very unlikely that this would interfere with any other piece
body > iframe:last-of-type {
@danielfdsilva Fixed, although I was not able to replicate this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, just a note so we know the quirks.
- the components that use the internal files (chart, image) don't work in the editor.
- the editor gets 'scrolled away' with scrolly map component.
but an exciting change! I look forward to getting some feedbacks.
bcd5977
to
7b0ef01
Compare
7b0ef01
to
fc37360
Compare