-
Notifications
You must be signed in to change notification settings - Fork 15
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
[UI] Unify export functions into one component #504
Conversation
apps/ui/src/components/Sidebar.tsx
Outdated
const [loading, setLoading] = useState(false); | ||
|
||
const onClick = () => { | ||
const exportSVG = () => { |
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.
I like the original approach that makes each button a separate React component.
- The original
exportSVG
component is short and easy to understand. - Now that you embed
exportSVG
into the longexportJupyter
component, you lose the easy-to-understand SVG logic, and also makes the Jupyter logic even harder to read.
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.
If we want to reuse code, we could abstract the common logic into a helper function (e.g., the downloadLink
function), and use it in both components.
In React, we compose components, not embed components.
Do this (compose):
<Parent>
<Child1/>
<Child2/>
</Parent>
Not this (embed):
<Parent>
<Child1AndChild2/>
</Parent>
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.
I think the right refactoring is to extract Jupyter export logic into a normal TS function and put it in a utils.ts
file.
apps/ui/src/components/Sidebar.tsx
Outdated
<ExportJupyterNB /> | ||
<ExportSVG /> | ||
</Stack> | ||
<Card elevation={1} sx={{ pl: "0", ml: "-20px", pb: "0", mb: "0" }}> |
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 Card
is too complex. Let's use Stack of Buttons like the original one:
<Stack spacing={1}>
<ExportJupyterNB />
<ExportSVG />
</Stack>
Thanks! |
Summany
Before
After