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

[Fleet] Cosmetic fixes #111725

Merged
merged 17 commits into from
Sep 14, 2021
Merged

[Fleet] Cosmetic fixes #111725

merged 17 commits into from
Sep 14, 2021

Conversation

juliaElastic
Copy link
Contributor

@juliaElastic juliaElastic commented Sep 9, 2021

Summary

Fixes for https://github.com/elastic/observability-design/issues/80
Should be all done now, see latest comments.

Bugs

  1. Wrong icon CTA in the Default policies screen
    Made Add Integration button filled
    image

Enhancements

  1. CTAs inconsistency

View screens
The Create/Add... button should always be filled
Actions should be unfilled
image
image

  1. CTA not displayed correctly

The UI of CTA in the Add agent flow looks broken

Julia: the right padding seemed missing, fixed that

image

  1. Warning button for Assign policy

Replace the warning button with a filled one.
image

  1. Consider replacing Copy policy with Duplicate policy/Make a copy

Using fleet for the first time I thought that Copy policy copied the policy to the clipboard because this is a pattern that I saw and I was familiar with. I think we should replace it with Duplicate, or if we want to keep Copy consider replacing the Copy policy CTA in the following modal with Make a copy or something similar

Julia: renamed button labels, though haven't renamed internal ids, let me know what you think. Changing ids might impact functional tests.
image

  1. Full stop missing

Add the full stop that is missing in the description (Fleet)
image

  1. Name field shows an error by default

Name field shows an error by default but the field is not mandatory and clicking on Create it will create a token giving a random name. This can be confusing. Consider adding a help text that explains what happens and why
image

Made Add integration button filled
Filled button fixes
3. Assign policy button
4. Duplicate policy
6. enrollment token name
@juliaElastic juliaElastic added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 9, 2021
@juliaElastic juliaElastic requested a review from a team as a code owner September 9, 2021 13:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@@ -86,7 +86,7 @@ export const AgentDetailsActionMenu: React.FunctionComponent<{
)}
<ContextMenuActions
button={{
props: { iconType: 'arrowDown', iconSide: 'right', color: 'primary', fill: true },
props: { iconType: 'arrowDown', iconSide: 'right', color: 'primary', fill: false },
Copy link
Member

Choose a reason for hiding this comment

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

nitpick do we need fill false or it's the default for the property?

placeholder="Enter a token name"
{...form.apiKeyNameInput.props}
/>
<EuiFormHelpText>Token id will be used when this is left empty.</EuiFormHelpText>
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to translate that you can use the <FormattedMessage> component for that.

Copy link
Member

Choose a reason for hiding this comment

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

Also I think we can define helpText= on the Form row component too https://elastic.github.io/eui/#/forms/form-layouts#form-and-form-rows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, will update

<EuiFieldText
name="name"
autoComplete="off"
placeholder="Enter a token name"
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to translate that you can use i18n.translate() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I wasn't sure where is it needed, as I don't see it used everywhere.
is there a way to find out which components translate out of the box?

Copy link
Member

Choose a reason for hiding this comment

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

No components translate out of the box, so if there is not translation we missed something previously

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Responsive fix fleet broken icons
@@ -91,7 +91,7 @@ export const AgentPolicyPackageBadges: React.FunctionComponent<Props> = ({
color="hollow"
isDisabled={excludeFleetServer && pkg.name === FLEET_SERVER_PACKAGE}
>
<EuiFlexGroup direction="row" gutterSize="xs" alignItems="center">
<EuiFlexGroup direction="row" gutterSize="xs" alignItems="center" responsive={false}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing broken fleet icons on small resolution
image

8. data streams table layout auto
@@ -202,6 +202,7 @@ export const DataStreamListPage: React.FunctionComponent<{}> = () => {
<EuiInMemoryTable
loading={isLoading}
hasActions={true}
tableLayout="auto"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line improves wrapping on data streams table - makes it less likely to wrap.
8. Text in the table should wrap correctly
image

responsive 2. fixed icon overlap
@@ -13,17 +13,10 @@ import type { UsePackageIconType } from '../../../../../hooks';
import { usePackageIconType } from '../../../../../hooks';
import { Loading } from '../../../../../components';

const PanelWrapper = styled.div`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for responsive layout (icon overlap) 2. Integration layout broken 480px, 768px breakpoints
See this comment https://github.com/elastic/observability-design/issues/80#issuecomment-916880833

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

I'm adding the 7.15.0 label so we can try to get these fixes in for the next build candidate for that release.

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀


@media (max-width: 767px) {
.header_container .euiFlexGroup--responsive > .euiFlexItem {
margin-bottom: 0 !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to reduce empty space in integrations header as suggested here https://github.com/elastic/observability-design/issues/80#issuecomment-917961482
image

const FormGroupResponsiveFields = styled(EuiDescribedFormGroup)`
@media (max-width: 767px) {
.euiFlexGroup--responsive {
align-items: flex-start;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix for form fields layout on add integration page (they were not wrapped in multiple lines)
image


@media (max-width: 767px) {
.euiFlexItem {
margin-bottom: 0 !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this to styled component, important is needed, because the original margin in eui has it too.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
fleet 1020.5KB 1021.1KB +633.0B

History

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

@juliaElastic juliaElastic merged commit b35b642 into elastic:master Sep 14, 2021
@juliaElastic juliaElastic deleted the cosmetic branch September 14, 2021 14:40
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 14, 2021
* [Fleet] Cosmetic fixes
Made Add integration button filled

* [Fleet] Cosmetic fixes
Filled button fixes

* [Fleet] Cosmetic fixes
3. Assign policy button

* [Fleet] Cosmetic fixes
4. Duplicate policy

* [Fleet] Cosmetic fixes
5. full stop

* [Fleet] Cosmetic fixes
6. enrollment token name

* [Fleet] Cosmetic fixes
2. auth settings

* [Fleet] Cosmetic fixes
review fixes

* [Fleet] Cosmetic fixes
Responsive fix fleet broken icons

* [Fleet] Cosmetic fixes
8. data streams table layout auto

* [Fleet] Cosmetic fixes
responsive 2. fixed icon overlap

* [Fleet] Cosmetic fixes
reduced empty space in header

* [Fleet] Cosmetic fixes
fix add integration form

* improve css selector

* moved css to styled component
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 14, 2021
* [Fleet] Cosmetic fixes
Made Add integration button filled

* [Fleet] Cosmetic fixes
Filled button fixes

* [Fleet] Cosmetic fixes
3. Assign policy button

* [Fleet] Cosmetic fixes
4. Duplicate policy

* [Fleet] Cosmetic fixes
5. full stop

* [Fleet] Cosmetic fixes
6. enrollment token name

* [Fleet] Cosmetic fixes
2. auth settings

* [Fleet] Cosmetic fixes
review fixes

* [Fleet] Cosmetic fixes
Responsive fix fleet broken icons

* [Fleet] Cosmetic fixes
8. data streams table layout auto

* [Fleet] Cosmetic fixes
responsive 2. fixed icon overlap

* [Fleet] Cosmetic fixes
reduced empty space in header

* [Fleet] Cosmetic fixes
fix add integration form

* improve css selector

* moved css to styled component
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.15
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 14, 2021
* [Fleet] Cosmetic fixes
Made Add integration button filled

* [Fleet] Cosmetic fixes
Filled button fixes

* [Fleet] Cosmetic fixes
3. Assign policy button

* [Fleet] Cosmetic fixes
4. Duplicate policy

* [Fleet] Cosmetic fixes
5. full stop

* [Fleet] Cosmetic fixes
6. enrollment token name

* [Fleet] Cosmetic fixes
2. auth settings

* [Fleet] Cosmetic fixes
review fixes

* [Fleet] Cosmetic fixes
Responsive fix fleet broken icons

* [Fleet] Cosmetic fixes
8. data streams table layout auto

* [Fleet] Cosmetic fixes
responsive 2. fixed icon overlap

* [Fleet] Cosmetic fixes
reduced empty space in header

* [Fleet] Cosmetic fixes
fix add integration form

* improve css selector

* moved css to styled component

Co-authored-by: juliaElastic <[email protected]>
kibanamachine added a commit that referenced this pull request Sep 14, 2021
* [Fleet] Cosmetic fixes
Made Add integration button filled

* [Fleet] Cosmetic fixes
Filled button fixes

* [Fleet] Cosmetic fixes
3. Assign policy button

* [Fleet] Cosmetic fixes
4. Duplicate policy

* [Fleet] Cosmetic fixes
5. full stop

* [Fleet] Cosmetic fixes
6. enrollment token name

* [Fleet] Cosmetic fixes
2. auth settings

* [Fleet] Cosmetic fixes
review fixes

* [Fleet] Cosmetic fixes
Responsive fix fleet broken icons

* [Fleet] Cosmetic fixes
8. data streams table layout auto

* [Fleet] Cosmetic fixes
responsive 2. fixed icon overlap

* [Fleet] Cosmetic fixes
reduced empty space in header

* [Fleet] Cosmetic fixes
fix add integration form

* improve css selector

* moved css to styled component

Co-authored-by: juliaElastic <[email protected]>
@jen-huang jen-huang added v7.15.1 and removed v7.15.0 labels Sep 20, 2021
@dikshachauhan-qasource
Copy link

dikshachauhan-qasource commented Oct 7, 2021

Hi @EricDavisX @juliaElastic

We have validated this PR UI related changes on 8.0 and 7.16 snapshot and found them working fine.

Build Details :

VERSION: 8.0 SNAPSHOT
BUILD 46587
COMMIT 9217be3287d9a4efa362900f8adcdd7aa7260459

VERSION: 7.16.0 SNAPSHOT
BUILD: 44811
COMMIT: 53fa6f53a005b244e70b5a696a8742d2fd1706f0

Further, observations made are as follows:

  • CTAs button are consistent under Fleet section>Agent tab/Policy tab/Enrollment token tab.
  • Warning button for Assign policy is filled now.
  • Full stop is available in the Fleet description.
  • Duplicate policy option is now displayed.
  • A help text is displayed under create enrollment token button.
  • Fleet icons working fine on small resolution.
  • Text in the table wraps correctly on small resolution.
  • Form fields layout on add integration page are not wrapped in multiple lines.

However, @juliaElastic Could you please share the lowest supported resolution with us so that we can validate is more precisely.

Thanks
QAS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.15.1 v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants