Conversation
| } | ||
|
|
||
| .sharing-ui-menu__ListWrapper, | ||
| .sharing-ui-menu__SharePanel { |
There was a problem hiding this comment.
These styles look great; I would just recommend sticking to all snake-case or all camel-case for consistency, instead of mixing them.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's on purpose, I usually do this when I have two elements that are inlined next to each other
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We may want to keep the is part of state classes, so that they are immediately indicative of component state. Thoughts?
There was a problem hiding this comment.
Totally agree. Let's use the pattern you gave here: https://github.com/elastic/kibana-ui-framework/pull/11/files#r72156938
|
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. |
`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>
This is the work in progress for the Sharing UI.
$scopeis flagged as$$destroyed, if passed to.registerkbn-share-navdirectivekbn-sharedirectiveui/kbn-sharetemplate, just like before this PRaction()when.register-ingPromiseor thenable telling us whether sharing succeeded, passed, or is asynchronousactionif no thenable is returnedHere's the UI when clicking Share in Discover.
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.
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.
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.
We can return a promise, in which case the nav bar is closed only after the promise is settled.
If the promise is rejected,
notify.errorcreates a notification atop the UI.