-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fs-103 View Report UI #43
Conversation
@@ -29,17 +31,19 @@ export const App = () => { | |||
<div className={styles.container}> | |||
{selectedMessage && ( |
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.
Once @mic-smith has done the grid work this will need to be conditional also on there being a report as it will be different for grid. I assume he can do that change later though
Looks good to me but I won't approve cause I worked on it too |
frontend/package.json
Outdated
@@ -45,6 +45,7 @@ | |||
"dependencies": { | |||
"classnames": "^2.5.1", | |||
"react": "^18.2.0", | |||
"react-dom": "^18.2.0" | |||
"react-dom": "^18.2.0", | |||
"react-markdown": "^8.0.6" |
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.
Any reason we don't have the latest version of react-markdown? e.g. "^9.0.1"
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 googled react markdown and the copied the install script from the npm page, hadn't realised that it wasn't the latest, good spot. @dianaPrahoveanu-SL would you mind updating it?
frontend/src/app.module.css
Outdated
align-items: center; | ||
justify-content: space-between; | ||
height: 56px; | ||
margin: -16px; |
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.
Any simple alternatives to negative margins here?
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'm working on this now. The reason I added it is to make sure the padding of the column element doesn't affect the report title. This way, the UI lines up with the UX specs.
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.
Couple of small comments, but looks good to me.
frontend/src/components/input.tsx
Outdated
setUploadedFile(file); | ||
appendMessage( | ||
{ answer: 'Your report is ready to view.' }, |
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.
Can we add the file name in, something like: "Your ESG report for file_name
is ready to view."
@@ -59,12 +61,12 @@ export const MessageComponent = ({ | |||
<img src={icon} className={styles.iconStyle} /> | |||
<p className={styles.messageStyle}>{content}</p> | |||
</div> | |||
{role == Role.Bot && false && ( | |||
{report && ( |
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.
We could potentially make this more generic, i.e. openSidePanel
with the button text variable being named openSidePanelButtonText
, this would prepare us for when the datagrid piece comes through - unless @mic-smith wanted to make this refactor in his own work?
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.
Yes, I'm happy to make this change in my PR as I need to edit this anyway
frontend/src/components/message.tsx
Outdated
@@ -18,6 +18,8 @@ export interface Message { | |||
content: string; | |||
reasoning?: string; | |||
time: string; | |||
report?: string; | |||
filename?: string; |
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.
Might be getting lost, is filename
used in here?
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.
Yes, on the app.tsx file I use it to pass the filename of the file uploaded to the title of the ESG report.
frontend/src/useMessages.ts
Outdated
response: ChatMessageResponse, | ||
role: Role, | ||
report?: string, | ||
filename?: string, |
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.
Possibly we could rename these to sidePanelContent
and sidePanelTitle
?
frontend/src/components/input.tsx
Outdated
{ answer: `Your ESG report for ${filename} is ready to view.` }, | ||
Role.Bot, | ||
report, | ||
filename, |
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.
Instead of passing in filename
, can we pass in `ESG Report - ${filename}`
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.
This will allow us to pass in a different title for the data grid, rather than having the component always add ESG Report -
in front of every sidePanelTitle
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.
Yeah, that makes sense. Will change now
frontend/src/components/message.tsx
Outdated
<div className={styles.selectMessage}> | ||
<Button | ||
isOutline | ||
isPressed={message === selectedMessage} | ||
text="Select message" | ||
text="View Report" |
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.
Can we also pass buttonText
in? This way Mike will be able to pass in View Data
or something like this
a279088
to
3ec3d5c
Compare
* FS-103 view report ui * Updated sidepanel title UI & linked report * PR comments- Mike * PR comments- Ivan --------- Co-authored-by: Emma Pearce <[email protected]>
Description
Displays the generated report from the uploaded file in the side panel.
Changelog