-
Notifications
You must be signed in to change notification settings - Fork 323
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
Vue entrypoint #11960
base: develop
Are you sure you want to change the base?
Vue entrypoint #11960
Conversation
🧪 Storybook is successfully deployed!📊 Dashboard:
|
}) | ||
} | ||
|
||
function imNotSureButPerhapsFixingRefreshingWithAuthentication() { |
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.
@somebody1234 I don't know what exactly the snippet below fixes. The Note is not clear to me.
I tracked that you are the original author, could you explain (and make a better name for this method :) )
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.
LGTM, left a few comments
// `style="all: unset"` breaking our layout. | ||
// See https://github.com/gloriasoft/veaury/issues/158 | ||
<div | ||
id="enso-dashboard" |
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.
Do we need enso-dashboard
wrapper though? We use this selector only in a few places - define base styles, and in utils, where we detect whenever an element should be hidden by click: app/gui/src/dashboard/components/AriaComponents/Dialog/utilities.ts
Guess we can safely migrate from enso-dashboard
to enso-app
and remove the wrapper
// See https://github.com/gloriasoft/veaury/issues/158 | ||
<div | ||
id="enso-dashboard" | ||
className={`${['App', 'enso-dashboard', ...props.classSet.keys()].join(' ')}`} |
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.
What's these classes stand for?
|
||
// Currently the GUI component needs to be fully rerendered whenever the project is changed. Once | ||
// this is no longer necessary, the `key` could be removed. | ||
return AppRunner == null ? null : <AppRunner key={appProps.projectId} {...appProps} /> | ||
return <ProjectView key={key} {...appProps} /> |
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.
For the future: can we wrap the editor into KeepAlive ?
We're looking into unmounting dom nodes when user switch tabs
Pull Request Description
Fixes #11185
Fixes #11923
I moved (and simplified) some setup. Mostly, the entry point sets up general behavior on page load, App.vue is the root, and the react-specific setups are now in
ReactRoot
component. The old App.vue is now just the ProjectView.vue.Removed some options which are no longer relevant since there's just a single GUI package.
I tried to put the ProjectView component into dashboard layout, but unfortunately, veaury is not helpful here, as it unsets all styles in its wrapper.
Also, GUI no longer displays Help view; if there's an unrecognized option we just print a warning and continue.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
[ ] Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
[ ] Unit tests have been written where possible.[ ] If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,or the Snowflake database integration, a run of the Extra Tests has been scheduled.