Skip to content

[wip] Sharing UI#7840

Closed
bevacqua wants to merge 2 commits intoelastic:masterfrom
bevacqua:feature/sharing-ui
Closed

[wip] Sharing UI#7840
bevacqua wants to merge 2 commits intoelastic:masterfrom
bevacqua:feature/sharing-ui

Conversation

@bevacqua
Copy link
Copy Markdown
Contributor

@bevacqua bevacqua commented Jul 25, 2016

This is the work in progress for the Sharing UI.

  • There's a service to register sharing functionality
  • Components register sharing options with that service
    • They're not shown when the $scope is flagged as $$destroyed, if passed to .register
  • Share button as a kbn-share-nav directive
  • Share options slide down via kbn-share directive
  • Component CSS is kept in ui/kbn-share
  • Sharing functionality
    • Able to define custom template, just like before this PR
    • Able to define custom action() when .register-ing
      • Can return a Promise or thenable telling us whether sharing succeeded, passed, or is asynchronous
      • Sharing panel is closed synchronously after action if no thenable is returned
  • Tests
  • Meets Pluggable sharing interface #7320 expectations
  • ...
  • Profit!

Here's the UI when clicking Share in Discover.

screen shot 2016-07-25 at 17 56 12

And here's when we click Link in the sharing options. Note that when there's more than a single sharing option, only the selected one is highlighted, and the "Back" button below it takes you back to the full list.

screen shot 2016-07-25 at 17 56 07

Here's an example with the code that can be found below copied into the Discover controller, it has a bunch more buttons that are example below.

screen shot 2016-07-25 at 18 03 39

Here's an example with a custom action. In this case, there is no extra configuration options, so once the "Download" button is clicked, the action is executed and then the top nav bar is closed.

kbnShare.register('discover.download', {
  $scope,
  icon: {
    title: 'Download',
    classes: 'fa fa-download'
  },
  action() {
    console.log('omg omg omg!');
  }
});

We can return a promise, in which case the nav bar is closed only after the promise is settled.

kbnShare.register('discover.delayed', {
  $scope,
  icon: {
    title: 'Slowly',
    classes: 'fa fa-clock-o'
  },
  action() {
    return new Promise(resolve => setTimeout(() => {
      console.log('yay!');
      resolve();
    }, 2000));
  }
});

If the promise is rejected, notify.error creates a notification atop the UI.

kbnShare.register('discover.failable', {
  $scope,
  icon: {
    title: 'Fail Soon',
    classes: 'fa fa-remove'
  },
  action() {
    return new Promise((resolve, reject) => setTimeout(() => {
      reject('omg omg failed!');
    }, 2000));
  }
});

@bevacqua
Copy link
Copy Markdown
Contributor Author

Please let me know if (besides the missing tests) this PR meets expectations set forth in #7320.

If not, what's missing?

/cc @epixa @uboness @Bargs

}

.sharing-ui-menu__ListWrapper,
.sharing-ui-menu__SharePanel {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These styles look great; I would just recommend sticking to all snake-case or all camel-case for consistency, instead of mixing them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So, that's what confuses me about BEM. I like doing, for example, .sum-share-panel where .sum means Sharing UI Menu. How do I differentiate between "this is the namespace" and "this is the component"? I reserved hyphen-case for states (.sharing-ui-menu__ListWrapper-is-open)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm... I think you should try treating the component as the namespace. For example, shareMenu:

/**
 * The shareMenu has a lot of children, indicated by the indented classes
 * below it.
 */
.shareMenu {
  text-align: center;
  display: inline-block;
  vertical-align: top;
}

  /**
   * This single indent indicates the child relationship. This class is tightly
   * coupled to the shareMenu. It literally can't be used outside of the
   * parent element... it just wouldn't make any sense in the UI. It has
   * the double-undescores because the shareMenu can't work without it either.
   */
  .shareMenu__list {
    display: block;
    margin: 0;
    padding: 0;
  }

    /**
     * This shareMenuItem is indented beneath the list for the same reason.
     * Its naming pattern changes because it's going to have child elements
     * and have something like .shareMenu__list__item__label__text will become
     * too verbose. This component has a little more identity that shareMenu__list
     * so it seems like a good place to re-establish its role as a parent of
     * child components, via its name.
     */
    .shareMenuItem {
      display: inline-block;
      text-align: center;
      padding: 10px;
    }

      .shareMenuItem__icon {
        display: block;
        font-size: 28px;
      }

/**
 * This is non-indented, because it's not a child of shareMenu. They're related
 * via the 'share' in their name, but it's clear they have different roles and
 * their relationship is distant, not direct.
 */
.sharePanel {
  display: inline-block;
  vertical-align: top;
  padding: 10px;
  width: ~"calc(100% - 150px)";
}

Want to Zoom about this tomorrow?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't notice this earlier, but sharing a styles declaration really tightly couples these two classes. This is great if there is some commonality or cohesion from the perspective of their role as UI components. But if it's just coincidence that .sharing-ui-menu__ListWrapper and .sharing-ui-menu__SharePanel share styles then I think it's better to duplicate the styles and split the classes apart. I think this will improve maintainability because then they can be changed independently without affecting one another.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's on purpose, I usually do this when I have two elements that are inlined next to each other

Copy link
Copy Markdown
Contributor Author

@bevacqua bevacqua Jul 26, 2016

Choose a reason for hiding this comment

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

I feel like we need a strong grip on what we want to do when it comes to naming classes.

As it stands, we're not going for a "lax" approach such as "just define a unique component namespace and do whatever you want in there", as in .abc-list, .abc-list-has-item, .abc-item, etc.

Having both a namespace, that's not clearly delimited, - because you could have say .shareButton and .shareButtonText - and at the same time having component names, is bound to be confusing.

Copy link
Copy Markdown
Contributor

@cjcenizal cjcenizal Jul 26, 2016

Choose a reason for hiding this comment

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

Per Zoom convo:

  • We'll use component names as namespaces (e.g. shareMenu) instead of abbreviated prefixes (e.g. sui-) because they're a little more concrete.
  • We'll use indentation to indicate parent-child relationships.
  • We'll change the pattern of state classes to use the component name first (I'll update the CSS style guide to reflect this).
  • We'll avoid changing components based on context because this makes the styles less maintainable. We'll also use component-specific state classes for the same reason.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We may want to keep the is part of state classes, so that they are immediately indicative of component state. Thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Totally agree. Let's use the pattern you gave here: https://github.com/elastic/kibana-ui-framework/pull/11/files#r72156938

@epixa
Copy link
Copy Markdown
Contributor

epixa commented Oct 8, 2016

I'm going to close this for now since @bevacqua is working on other things and we probably won't be getting to this particular sharing workflow for awhile.

@epixa epixa closed this Oct 8, 2016
jbudz added a commit that referenced this pull request Jun 28, 2024
`v95.1.0`⏩`v95.2.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v95.2.0`](https://github.com/elastic/eui/releases/v95.2.0)

- Updated `EuiContextMenuItemIcon`'s type definition to explicitly
define support for `EuiIcon`'s `IconType`
([#7804](elastic/eui#7804))
- Updated `EuiSteps` to support a new `titleSize="xxs"` style, which
outputs the same title font size but smaller unnumbered step indicators
([#7813](elastic/eui#7813))
- Updated `EuiStepsHorizontal` to support a new `size="xs"` style, which
outputs smaller unnumbered step indicators
([#7813](elastic/eui#7813))
- Updated `EuiStepNumber` to support new `titleSize="none"` which omits
rendering step numbers, and will only render icons
([#7813](elastic/eui#7813))
- Updated `setEuiDevProviderWarning` to additionally accept a custom
callback function, which warning messages will be passed to
([#7820](elastic/eui#7820))
- Updated `EuiIcon` to feature updated `logoElasticStack` logo for
referencing Elastic Stack platform
([#7838](elastic/eui#7838))
- Updated `EuiIcon` to feature updated `casesApp` design.
([#7840](elastic/eui#7840))
- Updated `EuiComboBox` to no longer autocomplete searched text when
used within forms ([#7842](elastic/eui#7842))

**CSS-in-JS conversions**

- Converted `EuiFilePicker` to Emotion; Removed
`$euiFilePickerTallHeight`
([#7833](elastic/eui#7833))

---------

Co-authored-by: Jon <jon@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants