style: implement Overview design#31
Conversation
stackingcats
left a comment
stackingcats
left a comment
There was a problem hiding this comment.
Starting to look good, thanks!
Some changes required, though.
| !status.status.includes("light mode") && status.status !== "Disabled" | ||
| ); | ||
| const getStatusIcon = (status: Status) => { | ||
| if (status.status.includes("Ready")) { |
There was a problem hiding this comment.
isServiceReady() would give a more accurate answer.
| }) | ||
| ); | ||
| const getCardHeaderBackgroundImage = (props: OverviewItemProps) => { | ||
| if (props.status.status.includes("Ready")) { |
There was a problem hiding this comment.
isServiceReady() would give a more accurate answer.
| ); | ||
| }; | ||
|
|
||
| const isDownloadLogsEnabled = (status: Status): boolean => { |
There was a problem hiding this comment.
By moving the Download Logs button inside the expanded view, there is a problem. As the expanded view is only visible when opendexd is up and running (since the information displayed is not available otherwise), the logs should still be available, no matter what the opendexd status is. Also, the logs should be available for any service that has a container, even if there's no additional information. The easiest solution would be to leave the Download Logs button in the main view. Otherwise in some cases the view should be expanded just to see the Download Logs button which is bizarre.
There was a problem hiding this comment.
I moved it back to the main view.
| </div> | ||
| )} | ||
| <ButtonBase onClick={handleClose}> | ||
| <img src={expandIcon} alt="expand" /> |
There was a problem hiding this comment.
Instead of the expand icon, there should be the "opposite" icon that indicates the collapse.
| <div className={classes.copyContainer}> | ||
| <ButtonBase onClick={() => copyToClipboard(value)}> | ||
| <img src={copyIcon} alt="copy" className={classes.copyIcon} /> | ||
| Copy | ||
| </ButtonBase> | ||
| </div> |
There was a problem hiding this comment.
The icon seems a bit off in two ways:
- When the ripple effect appears on click, there's no padding / no space around the text and icon;
- On the right of the icon, there's too much space. Would be good to give the content column (middle) more space.
| import { copyToClipboard } from "../../utils/appUtil"; | ||
| import IconButton from "../input/buttons/IconButton"; | ||
| import { copyIcon } from "../../../common/utils/svgIcons"; | ||
| import ButtonBase from "@material-ui/core/ButtonBase"; |
There was a problem hiding this comment.
There are button components in this project. Let's just use them in order to handle consistent style throughout the project.
| import Dialog from "@material-ui/core/Dialog"; | ||
| import DialogContent from "@material-ui/core/DialogContent"; | ||
| import { downloadIcon, expandIcon } from "../../common/utils/svgIcons"; | ||
| import ButtonBase from "@material-ui/core/ButtonBase"; |
There was a problem hiding this comment.
Same here regarding the custom button components.
| import { formatDateTimeForFilename } from "../../common/utils/dateUtil"; | ||
| import api from "../../api"; | ||
| import DialogTitle from "@material-ui/core/DialogTitle"; | ||
| import Dialog from "@material-ui/core/Dialog"; |
There was a problem hiding this comment.
There's also a Dialog component in this project. Would really make it lot easier to maintain to use this.
There was a problem hiding this comment.
I found it very annoying if I want to design the dialog title part then I will have to give like 3 classes as props, is it necessary to have internal Dialog component?
There was a problem hiding this comment.
Wouldn't it be an option to give the title as an input instead?
| <div> | ||
| <div className={classes.headingContainer}> | ||
| <h1 className={classes.heading}>Overview</h1> | ||
| <Icon |
There was a problem hiding this comment.
Please let's hide it until it has a functionality. Just some false condition here would do it.
Also, thinking of other views, they also need it so would not make sense to keep it in the Overview component but a PageTitleComponent or so would be meaningful.
There was a problem hiding this comment.
Created PageTitle component for it.
scorebreaker
force-pushed
the
design-overview
branch
from
dc7d8df to
4e4c4bf
Compare
…gs back outside Overview item, use collapse icon instead of expand, created PageTitle component to be reusable with all views
scorebreaker
force-pushed
the
design-overview
branch
from
4e4c4bf to
55df1a8
Compare
2 participants
Implement Overview design.