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

Minor frontend fixes #116

Merged
merged 15 commits into from
Jul 18, 2023
Merged

Minor frontend fixes #116

merged 15 commits into from
Jul 18, 2023

Conversation

maximi4321
Copy link
Contributor

Summary

This PR includes small changes that were pending to be implemented in the frontend.

Type of change

  • Bug fix.
  • Refactoring.

Changes

  1. Fixed the responsiveness in ResultsPage.jsx. The drawer no longer covers the table even if the screen width is changed.
  2. The browser's autofill functionality has been disabled for text input fields, because React has some problems handling autofill on controlled components
  3. A bug in the table for selecting runs to execute has been fixed (RunnerDialog.jsx). Now the user's selection is preserved (previously, all runs were reselected on any update).
  4. The modals for confirming the deletion of an item in the tables have been refactored. Since they were all very similar to each other, they have been replaced with a generic component.
  5. The API for editing datasets has been restored, which previously was not working due to compatibility issues with the backend.

How to Test

  1. Go to DashAI/DashAI/front and run yarn build
  2. Go back to DashAI/ and run python -c "import DashAI;DashAI.run()"
  3. Follow the steps in the "Screenshots" section.

Screenshots

Click to toggle the screenshots 1. Responsiveness in results page.

responsiveness

  1. Browser autofill is disabled

autofill

  1. Keeps the users run selection
Select.webm
  1. Generic modal to delete item

generic modal

  1. Restored edit dataset API
edit.dataset.webm

@maximi4321 maximi4321 added front Frontend work refactor Prune and embelish code labels Jul 7, 2023
@maximi4321 maximi4321 self-assigned this Jul 7, 2023
Copy link
Collaborator

@pbadillatorrealba pbadillatorrealba left a comment

Choose a reason for hiding this comment

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

The changes in general are fine!
I left some comments and questions.

DashAI/front/src/api/datasets.ts Outdated Show resolved Hide resolved
Comment on lines 197 to 206
<Paper
sx={{
p: 4,
[theme.breakpoints.up("xs")]: { ml: 8 },
[theme.breakpoints.up("sm")]: { ml: 12 },
[theme.breakpoints.up("md")]: { ml: 20 },
[theme.breakpoints.up("lg")]: { width: "80vw", ml: 15 },
[theme.breakpoints.up("xl")]: { width: "80vw", ml: 10 },
}}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Is the margin left assigned from the left edge of the image? I think it would be better if it was with respect to the drawer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the margin left is assing from the edge.

I am trying to define the margin with respect to the drawer, and I think I managed to do it. But by modifying that, now i haven't been able to make the table take up the entire space.

Copy link
Collaborator

@pbadillatorrealba pbadillatorrealba Jul 16, 2023

Choose a reason for hiding this comment

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

I think that Drawer is not the best component to use in this case (it is more intended to be the drawer where the navigation menu of the page is located) but List.

This will allow you to have the objects live in the same view and therefore, the size calculations are done with respect to the list and the table.

Suggestion: You could implement a view with two Grid, the one on the right with a small size where the List of experiments (ListItems) goes and the one on the left where the visualization goes.
And then make the grid sizes variable according to the size of the page using the typical sm, md, lg, etc...

DashAI/front/src/components/experiments/RunnerDialog.jsx Outdated Show resolved Hide resolved
@maximi4321 maximi4321 added the urgent Needs merge ASAP label Jul 14, 2023
Copy link
Collaborator

@pbadillatorrealba pbadillatorrealba left a comment

Choose a reason for hiding this comment

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

It looks good!

I think Container (in app.jsx) is the component that is causing the strange behaviors regarding the view responsive sizes.
For now, it's fine, but in the future we will have to think about how to deactivate it.

A minor margin change was requested.

Question: Could you disable the black background of the experiment list?

DashAI/front/src/pages/ResultsPage.jsx Outdated Show resolved Hide resolved
@pbadillatorrealba pbadillatorrealba merged commit b8867d5 into staging Jul 18, 2023
3 checks passed
@pbadillatorrealba pbadillatorrealba deleted the front/pending-work branch July 18, 2023 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front Frontend work refactor Prune and embelish code urgent Needs merge ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants