Feature/16 - Sidebar, Nodes, Node selection move to meta-diagram theme, add variables #17
Conversation
…es to meta-diagram
There was a problem hiding this comment.
@vidhya-metacell are you sure that the dist needs to be in git?
| cursor: "pointer", | ||
| } | ||
|
|
||
| const customNodeStyles = { |
There was a problem hiding this comment.
@vidhya-metacell why did you choose to use certain local object attributes for styling instead of using a material ui theme that contains the style/styles
There was a problem hiding this comment.
@zsinnema answered it here MetaCell/PsyNeuLinkView#2 (comment)
| const { style, engine, port } = this.props; | ||
| return ( | ||
| <> | ||
| <Button className="node-button"> |
There was a problem hiding this comment.
@afonsobspinto @vidhya-metacell I would like to see this refactored , this is not so DRY (see https://en.wikipedia.org/wiki/Don%27t_repeat_yourself)
example/index.tsx
Outdated
| const node1 = new MetaNode('1', 'node1', 'default', new Position(250, 100), | ||
| new Map(Object.entries({color: 'rgb(0,192,255)'}))) | ||
| const node1 = new MetaNode('1', 'Node 1', 'default', new Position(250, 100), | ||
| new Map(Object.entries(colorGreen))) |
There was a problem hiding this comment.
I would like this to be in the theme with theme provider like MUI
There was a problem hiding this comment.
@zsinnema if i add these variants to theme, then it limits us to use only specific variants like green, orange etc. I believe this should not be a restricted option, user should have an option to choose node color options from the project , also whatever color options he/she wants not just limited to the set theme options.
There was a problem hiding this comment.
I mean create an "application specific" theme and use that theme. If the app wants orange nodes then make if in the theme orange. For variants we can use this: https://mui.com/material-ui/customization/theme-components/#creating-new-component-variants
src/components/Sidebar.tsx
Outdated
| <ListItemButton | ||
| selected={selected === '1'} | ||
| onClick={() => setSelected('1')} | ||
| > | ||
| <ListItemIcon> |
There was a problem hiding this comment.
@vidhya-metacell please refactor into DRY, e.g. use an array with the listitems and then loop, factor out the listitem jsx into a seperate file
There was a problem hiding this comment.
@zsinnema Yes i agree, i will change this
| @@ -34,6 +34,16 @@ const vars = { | |||
|
|
|||
| switchShadow: | |||
| '0 0.1875rem 0.5rem rgba(0, 0, 0, 0.15), 0 0.1875rem 0.0625rem rgba(0, 0, 0, 0.06)', | |||
|
|
|||
There was a problem hiding this comment.
@vidhya-metacell do we need this if we do the MUI theming?
There was a problem hiding this comment.
@zsinnema Yes, as with other style properties. User can change the color/shadows whatever as required by the project.
There was a problem hiding this comment.
an instead of doing this let the user create a theme
@zsinnema yes @afonsobspinto mentioned we need to have an updated dist folder |
example/index.tsx
Outdated
|
|
||
| const node2 = new MetaNode('2', 'node2', 'default', new Position(500, 100), | ||
| new Map(Object.entries({color: 'rgb(255,192,0)'}))) | ||
| const node2 = new MetaNode('2', 'Node 2', 'default', new Position(500, 100), |
There was a problem hiding this comment.
@afonsobspinto @vidhya-metacell I suggest to add variant to the interface as a parameter (like position) instead of using options. This gives a better usability and clearer programmatic interface. I suggest that we do that for all parameters that we "currently" know.
There was a problem hiding this comment.
@afonsobspinto could you please add this to the node model.
|
While applying some changes requested by @vidhya-metacell I noticed the following bug was happening in this branch: It's not clear to me why it's happening like that in the example application, but I was told it's not happening on psyneulink |
# Conflicts: # dist/meta-diagram.cjs.development.js.map # dist/meta-diagram.cjs.production.min.js # dist/meta-diagram.cjs.production.min.js.map # dist/meta-diagram.esm.js.map
There was a problem hiding this comment.
@afonsobspinto in all files I'm missing the setter and getter for the new attribute 'variant'
also did you on purpose use == instead of ===
|
Since the getters and setters are non essential in the context of the current issue + considering the fact that I'm doing some model changes that drops the use of MetaOptions I purpose we handle this new request in a different issue @zsinnema . I did not put much thought in it. When using undefined I like to leave it flexible to also accept nulls but I'm ok with make it rigid. Just pushed a commit doing that. |
Closes #16