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

[Canvas] Toolbar UI Updates #113329

Merged
merged 5 commits into from
Oct 12, 2021
Merged

[Canvas] Toolbar UI Updates #113329

merged 5 commits into from
Oct 12, 2021

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented Sep 28, 2021

Summary

Related to #85666.
Blocked by #94139.

This integrates the SolutionToolbar component from presentation_util into the Canvas workpad header and aligns the UI for adding content in Dashboard and Canvas.

Before:

Screen Shot 2021-10-05 at 6 00 23 PM

After:

Screen Shot 2021-10-05 at 6 01 51 PM

Changes:

  • Adds SolutionToolbar component to WorkpadHeader component
  • Replaces Add from Kibana menu option nested in the Add element menu with Add from library button on the toolbar
  • Adds quick buttons for Text, Shape, and Image elements

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cqliu1 cqliu1 added enhancement New value added to drive a business result Feature:Canvas release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort v8.0.0 labels Sep 28, 2021
@cqliu1 cqliu1 changed the title [Canvas] Integrates Solution Toolbar [Canvas] Toolbar UI Updates Sep 29, 2021
@cqliu1 cqliu1 force-pushed the canvas/panel-toolbar branch 2 times, most recently from b413cb7 to 9c27a9e Compare October 4, 2021 17:55
@cqliu1 cqliu1 added the review label Oct 5, 2021
@cqliu1 cqliu1 marked this pull request as ready for review October 5, 2021 18:20
@cqliu1 cqliu1 requested review from a team as code owners October 5, 2021 18:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

I think the colors of the buttons on the left should match the one on the right, so you'd have to give that svg's fill the same color that <EuiButton color="text"> uses
Frame 22

Fixed ts errors

Fixed proptypes

Fixes tests
@cqliu1
Copy link
Contributor Author

cqliu1 commented Oct 6, 2021

Screen Shot 2021-10-05 at 6 08 46 PM

I'm not setting a color, so it should default to text. It looks like the EuiButtonGroup component is applying a slightly different color.

The same goes for dark mode

Screen Shot 2021-10-05 at 6 16 36 PM

@cqliu1 cqliu1 force-pushed the canvas/panel-toolbar branch from 9c27a9e to fde83c0 Compare October 6, 2021 01:27
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Seems like the different shades of gray are coming from EUI. I can check with them but not a blocker for this. LGTM

@cqliu1 cqliu1 force-pushed the canvas/panel-toolbar branch from fde83c0 to 5bd8a53 Compare October 6, 2021 18:18
Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks good to me and functions like expected.

Some feedback that I'm sure has been discussed elsewhere, do we like having a quick button for Text/Image/etc and also having them in the dropdown?

...stateProps,
...dispatchProps,
...ownProps,
// Moved this section out of the main component to enable stories
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this comment. It keeps getting moved around as the code moves, so I don't think it's really necessary.

@cqliu1
Copy link
Contributor Author

cqliu1 commented Oct 12, 2021

Some feedback that I'm sure has been discussed elsewhere, do we like having a quick button for Text/Image/etc and also having them in the dropdown?

I'd prefer to leave the options in the element menu for discoverability. IMO, it'd be helpful to have all the element options in one list at least for new Canvas users even if it's redundant to have the quick buttons as well.

@andreadelrio what do you think?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
presentationUtil 151 150 -1

Async chunks

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

id before after diff
canvas 1.0MB 1.0MB -218.0B
dashboard 135.4KB 135.4KB -34.0B
total -252.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
presentationUtil 43.6KB 43.0KB -592.0B
Unknown metric groups

API count

id before after diff
presentationUtil 178 177 -1

History

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

@andreadelrio
Copy link
Contributor

Some feedback that I'm sure has been discussed elsewhere, do we like having a quick button for Text/Image/etc and also having them in the dropdown?

I'd prefer to leave the options in the element for discoverability. IMO, it'd be helpful to have all the element options in one list at least for new Canvas users even if it's redundant to have the quick buttons as well.

@andreadelrio what do you think?

++. I think it makes sense to list all available elements under + Add Element and have that co-exist with the quick buttons.

It’s similar to what Google Slides does where there are quick buttons for Shape, Image and Text which users can also find under Insert > Shape, Insert > Image and Insert > Text.

@cqliu1 cqliu1 merged commit 16c049a into elastic:master Oct 12, 2021
@cqliu1 cqliu1 deleted the canvas/panel-toolbar branch October 12, 2021 18:53
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 14, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

3 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

3 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

4 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113329 or prevent reminders by adding the backport:skip label.

@spalger spalger added the backport:skip This commit does not require backporting label Nov 1, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 1, 2021
@cqliu1 cqliu1 restored the canvas/panel-toolbar branch June 8, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting enhancement New value added to drive a business result Feature:Canvas impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort release_note:enhancement review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants