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

[ML] Moves import buttons in file data visualizer #159722

Merged
merged 10 commits into from Jun 20, 2023

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Jun 14, 2023

Relates to #112285

Removes the black bar at the bottom of the app which housed the import and cancel buttons.

Rather than having a button called "Change import settings" I've left this as the "Back" button. "Change import settings" isn't really a correct description of the page the button returns you to. Something like "Rerun analysis of the already selected file" would be accurate, but that is too long for a button.

@mdefazio Does this look ok? Any other suggestions? Note, I'd like to keep this PR small, the main aim was just to remove the black bar at the bottom of the page and change the name of the Cancel button after the import has already run.

image

image

image

@jgowdyelastic jgowdyelastic marked this pull request as ready for review June 15, 2023 08:00
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner June 15, 2023 08:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@@ -67,6 +74,19 @@ export const ResultsView: FC<Props> = ({
<EuiSpacer size="m" />

<EuiFlexGroup gutterSize="s" alignItems="center">
<EuiFlexItem grow={false}>
Copy link
Contributor

Choose a reason for hiding this comment

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

The bottom bar used to have a 'Cancel' button on it which took you back to the entry point of the file data viz. Now the only way to go back and import another file (before importing) is to use the breadcrumbs or hit the browser back button. What do you think about adding a 'Select a different file' type button into here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a button next to the file name title

image

Copy link
Contributor

Choose a reason for hiding this comment

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

The link you add to 'Select a different file' LGTM

image

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM.

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

fileUpload={this.props.fileUpload}
getAdditionalLinks={this.props.getAdditionalLinks}
capabilities={this.props.capabilities}
mode={MODE.IMPORT}
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ shall we pass the mode variable instead of the constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 5e16356

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

@jgowdyelastic Excited to see movement on this! Just a few small comments really:

Is there a background behind the content here? I know we had one in the mockup, but for this first iteration, I don't think it's needed with the page setup as it is.
image

I can imagine there could be confusion between 'Back' and 'Cancel' within this context. What about changing 'Cancel' to Cancel upload so it's clearer? When 'Cancel' was next to 'Import' in the bottom bar it made sense as is. But with this move, might be better to add that extra clarification.
image

I think that's it for this first step. I'd be happy to revisit the mockups and work through some feedback that way for future iterations if it helps.

@jgowdyelastic
Copy link
Member Author

jgowdyelastic commented Jun 20, 2023

@mdefazio I hadn't noticed that the background color behind the file name on the import page is a different to the previous page.
When this feature is embedded in ML the background should be white to match all of our other pages, when embedded in the Add Data page, the background should be off-white
e.g.

image

vs

image

I'll fix that title in this PR.
Changing the background for the whole of the page would make it look very different to the rest of ML.

@mdefazio
Copy link
Contributor

Changing the background for the whole of the page would make it look very different to the rest of ML.

Agreed! Didn't mean to imply we should change the whole background—just that it could be removed so it matches the rest of the page. Thank you!

@jgowdyelastic
Copy link
Member Author

jgowdyelastic commented Jun 20, 2023

@mdefazio

What about changing 'Cancel' to Cancel upload so it's clearer?

Sorry, the screenshot in the PR description was out of date, the Cancel button is now Select a different file

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

LGTM.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataVisualizer 346 344 -2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dataVisualizer 367.8KB 367.3KB -470.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 411 415 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 494 498 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jgowdyelastic

@jgowdyelastic jgowdyelastic merged commit ca0b739 into elastic:main Jun 20, 2023
21 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 20, 2023
@peteharverson peteharverson changed the title [ML] File data visualizer moving import buttons [ML] Move import buttons in file data visualizer Jun 21, 2023
@peteharverson peteharverson changed the title [ML] Move import buttons in file data visualizer [ML] Moves import buttons in file data visualizer Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants