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

Feature/sof 6727 #127

Merged
merged 67 commits into from
Aug 2, 2023
Merged

Feature/sof 6727 #127

merged 67 commits into from
Aug 2, 2023

Conversation

VsevolodX
Copy link
Contributor

Draft PR

Added TS, imported cove.js package and necessary packages for it to work, viewSettingsToolbar is moved to Dropdown and actions work,
probably broke that one when updating the cove.js locally
.eslintrc.json Outdated Show resolved Hide resolved
"underscore.string": "^3.3.4"
"underscore.string": "^3.3.4",
"@exabyte-io/cove.js": "2023.7.14-0",
"@mui/lab": "^5.0.0-alpha.120",
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

@VsevolodX VsevolodX Jul 17, 2023

Choose a reason for hiding this comment

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

It was a solution to import cove.js, from error messages those libraries were missing.

Without them:
image

They are peer dependencies in cove.js:
image

Copy link
Member

Choose a reason for hiding this comment

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

@zoomchik - do we need this? Any way to avoid?

Copy link
Contributor

Choose a reason for hiding this comment

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

@timurbazhirov We need this packages as peerDependencies at least to avoid multiple copies of React. https://legacy.reactjs.org/warnings/invalid-hook-call-warning.html#duplicate-react

import { WaveComponent } from "./WaveComponent";

const darkTheme = createTheme({
Copy link
Member

Choose a reason for hiding this comment

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

We should use the cove.js theme

fullWidth: boolean;
}

export const DefaultDropdownButton = function ({
Copy link
Member

Choose a reason for hiding this comment

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

Why do we even need this component? It's just using the default Button

/**
* MUI dropdown component have a default button with dropdown also could be used with
* custom button which takes from children, actions array -> array which will be converted
* to dropdown menu items.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is not understandable, plus has many grammatic errors

Copy link
Contributor Author

@VsevolodX VsevolodX Jul 24, 2023

Choose a reason for hiding this comment

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

Sure it is. I've noticed it too, but this was just a test so I just copied to use the component.

{...popperProps}
>
{({ TransitionProps }) => (
<Grow {...TransitionProps} style={{ transformOrigin: "center top" }}>
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need all this stuff?? There's a Dropdown component in cove.js that we should update to deal with the necessary logic

{...popperProps}
>
{({ TransitionProps }) => (
<Grow {...TransitionProps} style={{ transformOrigin: "center top" }}>
Copy link
Member

Choose a reason for hiding this comment

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

Code here is exactly the same as in the component above - remove duplicates

@@ -0,0 +1 @@
/// <reference types="react-scripts" />
Copy link
Member

Choose a reason for hiding this comment

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

WTH is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was created during npm run start. I'll look into tsconfig.json, how it's handled in other packages.

import Replay from "@mui/icons-material/Replay";
import Spellcheck from "@mui/icons-material/Spellcheck";
import SquareFootIcon from "@mui/icons-material/SquareFoot";
import SwitchCamera from "@mui/icons-material/SwitchCamera";
import ThreeDRotation from "@mui/icons-material/ThreeDRotation";
import { createTheme, ThemeProvider, Tooltip } from "@mui/material";
// import { styled } from "@mui/system";;
import { ButtonGroup, createTheme, ThemeProvider, Tooltip } from "@mui/material";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not import the whole lib, import individual components eg. import from "@mui/material/Tooltip";

@VsevolodX VsevolodX marked this pull request as ready for review August 1, 2023 18:56
@@ -68,6 +67,7 @@ function ParametersMenu(props: ParametersMenuProps) {
className="inverse stepper cell-repetitions"
id={`repetitionsAlongLatticeVector${label}`}
value={
// @ts-ignore
(viewerSettings as { [key: string]: any })[
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use ViewerSettings type above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zoomchik with help from Alex I went with defining type of key:

type ViewerSettingsKey = keyof ViewerSettings;
const key = `repetitionsAlongLatticeVector${label}` as ViewerSettingsKey;

@VsevolodX VsevolodX merged commit 95873b2 into dev Aug 2, 2023
2 checks passed
@VsevolodX VsevolodX deleted the feature/SOF-6727 branch August 2, 2023 21:56
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.

None yet

3 participants