Hey! 👋

Getting an error from husky when trying to push. Any ideas? Did anyone had the same before?

husky > pre-push (node v13.11.0)
$ d2-app-scripts test
Running tests... 
FAIL src/__tests__/StackedTable.test.js
  ● Test suite failed to run
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main resolved in /Users/user/dev/dhis2/ui-core/node_modules/@babel/helper-compilation-targets/package.json

  at Object.<anonymous> (node_modules/@babel/preset-env/lib/debug.js:8:33)
  at Object.<anonymous> (node_modules/@babel/preset-env/lib/index.js:11:14)

Test Suites: 1 failed, 1 total

Tests:       0 total

Snapshots:   0 total

Time:        0.996s

Ran all test suites.

[ERROR] Tests failed

error Command failed with exit code 1.

info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

husky > pre-push hook failed (add --no-verify to bypass)

Hook process exited.

See this thread: https://dhis2.slack.com/archives/CBM8LNEQM/p1589961187000500

Two related problems:

1. Some of our field components have a margin-right to space the input and a potential icon:

• https://github.com/dhis2/ui-core/blob/05512df65f1a7f59dc4ee38b28927c1514896658/src/SingleSelect.js#L110
• https://github.com/dhis2/ui-core/blob/05512df65f1a7f59dc4ee38b28927c1514896658/src/MultiSelect.js#L110 (and maybe others)

This margin is always present, even if there's not an icon. We should make it conditional and not set a margin if there's no icon.

2. In the input we use a wrapper for the status-icon, which always renders a margin-left, throwing off alignment as well.

We should make this conditional as well, and should probably use the same approach everywhere. (Could be present in other components as well btw.)

https://github.com/dhis2/ui-core/blob/05512df65f1a7f59dc4ee38b28927c1514896658/src/Input.js#L68

See here: https://dhis2.slack.com/archives/CBM8LNEQM/p1589962613009000?thread_ts=1589961187.000500&cid=CBM8LNEQM

Follow up for this PR: #61, in which we added default translations for some of our widgets props.

• The FileInput has two other props that could still use translations: Cancel link (Cancel) & Loading state (Uploading)
• Potentially the Transfer could also use translations

The way we stack layers needs to be revisited. It is purely internal so there should be no breaking changes.

The raison d'être for the layer context is that we need to make sure that the components that display "above" their own base layer, for example: the Select Menu, DropMenu, Popover, etc.

These components should appear on the application top layer. Just setting these components to z-index: 2000 works as long as they are on the application layer (0-1999).

However.

What if a Select is rendered inside of a Modal, that exists on the blocking layer (3000)?

With a naive solution, the Select Menu would render with a zindex: 2000 and subsequently render beneath the Modal it sits on.

Current situation

We solved nested layer stack problem with a Provider so that a child can "look up" to its nearest Provider to see what the z-index on the parent is. Then the Layer component can calculate a new z-index and return a new Provider boundary.

If a blocking layer is used, then we increment by 2 to account for an eventual child that needs to render "on top" of the blocking layer.

If a non-blocking layer is used we increment the z-index by 1 to get in between the first blocking layer and the second blocking layer, e.g. the Modal 1 Select Backdrop renders above Modal 1 but below Modal 2.

This approach is limited in that regard.

Select Backdrop: 2000
    Select Menu: auto

Custom Select Backdrop: 1000
    Select Menu: auto

Modal 1: 3000
    Modal 1 Select Backdrop : 3001
        Modal 1 Select Menu: auto

Modal 2: 3002
    Modal 2 Select Backdrop : 3003
        Modal 2 Select Menu: auto

Modal 3: 3004
    Modal 3 Select Backdrop : 3005
        Modal 3 Select Menu: auto

History

We have implemented a system that works for now but noone is overly excited about it. It seems needlessly complicated to use Portals and recursively figuring out what z-index to use. We think we can do better, so we have kept the LayerContext implementation internal to the library so we can refactor without causing breaking changes.

This way we can proceed with dhis2/ui-core#430 and allow ourselves some headspace to think about the layer stacking without the pressure of a looming major release.

• dhis2/ui-core#582
• dhis2/ui-core#590
• dhis2/ui-core#593

dhis2/ui-core#743 adjusted the arrow icon for dropdown/split buttons. The icon is also being used in the Node component. The icon is now misaligned in the Node.

@martinkrulltott encountered an issue where the Select rendered below the Modal. We have encountered this issue before and thought we had solved it by using context (React.createContext()). However, there seems to be a case where this falls apart:

• The data-visualizer-app is dependent on the analytics package
• Both data-visualizer and analytics have their own instance of ui-core.
• As such the different layers and layer-contexts seem to be fully unaware of each other
• And as a result the Select (from analytics) uses its default zIndex value because it doesn't know it's stacked on top of another layer

Martin provided a screen recording on Slack that demoes the issue very well. You can see in the screen-recording that the Select which is coming from the same ui-core instance as the Modal (both data-viz) is working fine and is on top of the Modal (Select gets z-index 3001), but the one coming from analytics is misbehaving and gets rendered below the Modal (it just used a z-index of 2000)....

I didn't get a chance to debug this properly, but going by what we are seeing in the screen-recording, I can't really think of another explanation. This isn't necessarily an urgent issue, but it does seem to be quite a fundamental one... I don't really know how to tackle it.

And this issue is relevant to the components I am working on at the moment, see #10

The parts I noticed:

1. We are moving away from { value, label } objects for selected values and instead using string values.

2. The multiselect proptypes is gone, there's a TODO where I've copied in the old shared prop types for multiselect, see point 1.

3. Transfer uses InputField, so technically it cannot be a core component. The choice was between moving Transfer to widgets or refactoring to use Field+Input. I did the latter in this PR, but maybe we want it to be a widget (with translations, e.g.)

4. I'm not fond of the common pieces. I had a hard time keeping track if I was looking at Transfer/__e2e__/common.js, __tests__/common/*, or Transfer/common.js.

5. Please specify the extension when importing (this is the code style in UI !). Using import { foo } from 'directory' or import { foo } from 'file' to import from directory/index.js or file.js is not ES compatible -- it is a Node-ism that is supported by Webpack. Pure ES tools do choke on that unless it is Babelified or handled later. Would be nice to have a ESLint for this, as it makes it a lot easier to understand exactly where the code is imported from.

6. The __tests__/common has several options:

• Add a private package to packages/test-utils, then we can import createChildren from ui-test-utils (I like this one).
• Create a folder in packages/core/src named utils and put createChildren.js there.
• Don't automate this.

Originally posted by @varl in #49 (comment)

Follow up from: #46 (comment)

In our stories we sometimes use the title Default, or similar titles, which aren't very descriptive. It's easy for such a story title to get out of sync with what it's actually meant to contain, especially if it's reused in multiple test scenarios. A similar thing goes for our step definitions.

I'm in favour of stating exactly what goes on in a story, or step (and in the case of a step, asserting the statements made in that step), to prevent title and content drifting apart.

So if a story prepares a test environment with multiple properties (for example: red, and highlighted, and active, etc.), I'm in favour of listing all of that in a Single Given if it's all prepared in a single step definition (loading the story). I'm only in favor of breaking it out into Given's and And's if removing that step from the feature will also remove the property from the test environment (so for example if removing And is selected will also mean that it'll no longer be selected in the test).

Otherwise it's hard to reason about the feature files, step definitions or stories in isolation, and you can only work on them when you understand how they interact. I'd prefer devs to only have to think about the current file they're working on, seems easier to not make any mistakes that way.

The popper.js developers have introduced react-popper v2, and unlike v1, this looks really great. It would make a lot of sense to start using that for our Popper.

A ui-forms bugfix. We should update final-form to 4.19.0+ and react-final-form to 6.4.0+ to address a warning caused by a setState that does not follow the recommendations of the latest react version.

See this issue: final-form/react-final-form#751

The Transfer component can only display a header in the source list, there should also be the option of displaying a header in the picked list. For example:

The header design is the same as the source/left-side, it accepts any type of content and has the same default padding. The header is optional.

https://jira.dhis2.org/browse/TECH-394

When handling a large amount of items in the Transfer component there's a need to support infinite scrolling, to be able to initially load a subset of data and sequentially load more when scrolling. Details: https://jira.dhis2.org/browse/TECH-378

We've added a few things to the Popper and Popover which should be tested:

• Accepting different types of variables for the reference prop, i.e. React.ref, DOM node, and virtual element
• Arrow directions for the popover. We should test all 4 main directions (top/bottom/left/right) + complex placement (i.e. bottom-start) + verify that the arrow shifts a little bit when the reference element is narrower than the popper element.

5.0.0 scope

• Co-locate UI libraries in repo dhis2/ui
• Introduce the @dhis2/ui top-level package and main entry-point for the UI libraries.
• Keep @dhis2/ui-core and @dhis2/ui-widgets as-is:
• • core: Ready-to-go building blocks for UI
• • widgets: Components that need translations and/or DHIS2 API
• Internal split to:
• • @dhis2/constants
• • @dhis2/icons

Upgrade path from 4.x to 5.x

• Generally all imports from either @dhis2/ui-core, @dhis2/ui-widgets, and @dhis2/ui-forms can be replaced by a single namespace: @dhis2/ui.
• There are some cases where we have rename a component to avoid name conflicts.
• Some components have been dropped, see the CHANGELOG for more information.

All of our components that have a clear "root-element" should be wrapped in React.forwardRef, and apply a ref to that root element. This would be a good thing in general, for various use-cases in apps, but specifically for allowing our components to be used as anchor-elements for popper-based components.

ButtonStrip adds margins to child Buttons, but not to child SplitButton.

Adds margin (child Button)

Doesn't add margin (child SplitButton)

Correct spacing (child Button)

Incorrect spacing (child SplitButton)

This issue was created as a result of this #66 (comment). It would be good to start using less helpers in our tests code. I know that quite a lot of components have a position.feature file, and all of these make quite extensive usage of a helper. So that's one part that would definetely have to be addressed, but quite likely there's more.

I was only able to reproduce this on FF (on macos). After resizing the browser window to the left, with the select dropdown open, sometimes the dropdown is sometimes positioned too far too the left (16px):

Hendrik and I looked at this for a moment, but weren't able to find out what exactly's causing this. We were thinking that it might be the event listeners and resize observer competing (or the rerenders due to the changed menuwidth interfering). There seems to be some sort of race condition somewhere, because it doesn't occur every time.

I've just tried to have a proper look at the ui@alpha repo and I have a few questions.
I'll use this isseu to collect questions and put the answers/discussion outcome into this issues's description

Resolved questions

none yet

🚨 The automated release from the alpha branch failed. 🚨

I recommend you give this issue a high priority, so other packages depending on you could benefit from your bug fixes and new features.

You can find below the list of errors reported by semantic-release. Each one of them has to be resolved in order to automatically publish your package. I’m sure you can resolve this 💪.

Errors are usually caused by a misconfiguration or an authentication problem. With each error reported below you will find explanation and guidance to help you to resolve it.

Once all the errors are resolved, semantic-release will release your package the next time you push a commit to the alpha branch. You can also manually restart the failed CI job that runs semantic-release.

If you are not sure how to resolve this, here is some links that can help you:

• Usage documentation
• Frequently Asked Questions
• Support channels

If those don’t help, or if this issue is reporting something you think isn’t right, you can always ask the humans behind semantic-release.

Cannot push to the Git repository.

semantic-release cannot push the version tag to the branch alpha on the remote Git repository with URL https://[secure]@github.com/dhis2/ui.git.

This can be caused by:

• a misconfiguration of the repositoryUrl option
• the repository being unavailable
• or missing push permission for the user configured via the Git credentials on your CI environment

Good luck with your project ✨

Your semantic-release bot 📦🚀

Currently submitErrors passed to our react-final-form compatible form components trigger the error state, but the submitError message itself isn't displayed.

We'll need to handle submitError here as well to enable the above: https://github.com/dhis2/ui/blob/alpha/packages/forms/src/shared/helpers/getValidationText.js#L4 (and there might be other changes necessary that I'm overlooking now).

The <ComponentCover /> component has css with width: inherit and height: inherit. This works well if the parent of the cover has explicit width and height set, bit it breaks when the parent is dynamically sized - for instance, if the parent is the child of a flex container.

Is there reason inherit was chosen and could we perhaps support another mechanism to cover dynamically-sized parent elements? In many cases width: 100%, height: 100% may work, but that could have problems determining the proper height.

Continuing the discussion from dhis2/ui-core#457

Is this in addition to the "Inline label" that's specced in: https://github.com/dhis2/design-system/blob/master/molecules/select.md#inline-label?

And, could the "inline label" extend to the InputField in the future?

Good point @varl. I actually don't think we need both 'left positioned label' and 'inline label', the use cases overlap too much. I included the 'inline label' option for select because I wanted to offer an out-of-the-box 'compact' solution for table headers and such spaces, where it's quite important a default select isn't used. A left positioned label would solve for that too.

Therefore, if we offer position:left for labels I think we can remove the 'inline-label' option on the select. I assume this would simplify the build of the select field too, and we avoid the awkward question you raised: why isn't 'inline label' available for input too?

Clicking Re-run jobs for the cypress GH actions breaks with the following error:

See here for example: https://github.com/dhis2/ui/pull/66/checks?check_run_id=674857570

I've run into some bugs with the Select, and suspect that the old menu positioning logic and the new popper driven logic are clashing a bit. We've had an integration test for the Select fail randomly as well, so I want to take a good look at it and see if we can resolve the flakiness and ensure everything is working as expected.

When I was implementing the FileInput in ui-forms I realised that currently it's not possible to attach onBlur and onFocus to the ui-core FileInput. The reason for this is that the only visible UI element is a button (i.e. our Button component), and you can't focus the hidden file input programatically.

There are some ways to "hide" the file input in a different way, i.e. by giving it width and height and opacity 0. That way it would be possible to programatically set focus to this element. But I think that doesn't really solve the problem, because the visible UI component is the button so that should keep the focus for good UX.

So I propose we add an onFocus and onBlur prop to the Button instead.

Various components handle className differently. Ideally there should be a way to target the top level parent element of the rendered output of the component.

Case examples:

SingleSelect and MultiSelect

SplitButton

ToggleGroup / RadioGroup - applies to the children, leaving an unstylable div at the top level.

Storybook now recommends implementing each story as a named function that returns JSX. This makes it a component. Our ESLint rule react/prop-types states that every component should have propTypes declared. This can be very inconvenient for stories that get passed a prop from a decorator.

The suggested solution is to leverage the cascading nature of ESLint configuration and to place an .eslintrc.json file in ./src/__demo__ and ./src/__e2e__, with the following content:

{
    "rules": {
        "react/prop-types": [1, { "skipUndeclared": true }]
    }
}

This solution seemed to work to an extend:

• My VSCode ESLint plugin was picking up this rule
• When running yarn lint the rule was applied too
• But running yarn cy:open was ignoring this rule.

I suggested that the problem with yarn cy:open could have something to do with having EXTEND_ESLINT=true in the following scripts entry in package.json:

"start:testing": "EXTEND_ESLINT=true STORYBOOK_TESTING=1 start-storybook --port 5001 --quiet --ci",

However, this could also just be a CRA related thing.... So not sure how to fix this part.

Please see this Slack thread for the full discussion.

Dependabot was set up to create pull requests against the branch master, but couldn't find it.

If the branch has been permanently deleted you can update Dependabot's target branch in the .dependabot/config.yml file in this repo.

View the update logs.

When searching using a filter in the Transfer component there's a need to display a loading spinner while the result is getting fetched. Details: https://jira.dhis2.org/browse/TECH-379

We currently have e2e stories defined in core, widgets and forms. In cypress/integration we have a forms folder that contains the feature files and step definitions for the forms stories. But for core and widgets those are all mixed at the root of cypress/integration.

Once everything has settled after the move it'd be nice to have uniform organisation in this folder. I'd propose to organise all our step/feature files the way the forms ones are organised, so:

cypress/integration
  - forms
  - core
  - widgets

Currently we have several components with overlapping responsibility and some duplication. Some things I noticed:

• ScreenCover uses a Backdrop, but it’s a private one, not the one that we expose..
• Suppose the (public) Backdrop is meant to be placed into a ScreenCover or ComponentCover (I think it should), then I think the styling it has is way too opinionated, it should just cover the entire parent, not work with a position: fixed.
• The ScreenCover is rendering its children into a Content component, which vertically and horizontally centers its content. Again, I think this is too opinionated.
• The ComponentCover does something similar

So I’d like to propose, for the next major release to change things around a bit:

• Layer covers the window or body and takes care of stacking (z-index), clicks are not captured by this component, so it is not a "blocking" element.
• ComponentCover covers the component, provided the parent has a non-static position, , clicks are not captured by this component, so it is not a "blocking" element.
• Backdrop: can accept an onClick, has a semi-transparent background by default and a transparent prop. This component just takes up 100% width and height of the parent so it can be used to make a Layer or ComponentCover "blocking".
• CenteredContent: aligns its content vertically and horizontally.

If we structure the components like this, it's pretty easy to build a variety of blocking/non-blocking overlays with/without loaders (or other centered content)

edited by @HendrikThePendric on 10-3-2020

Our cypress tests have been a bit flaky lately. I think this is partially due to our usage of cypress' .then instead of .should. The cypress docs recommend using .should as it'll automatically retry:

As they note, we should be aware that the callback in a .should could be run multiple times, so we should make sure that there are no side effects in the callback.

So I'd say:

• If it's an assertion, use .should instead of .then (make sure there are no side-effects)
• If you want to work with the subject yielded from the previous command, and it's not an assertion, use .then

If everyone agrees I say we refactor our tests to follow the above.

Currently the validation error for the createNumberRange validator displays 'Please enter a number between {{lowerBound}} and {{upperBound}}', but the validation allows lowerBound and upperBound as well, so we should include those number too.

I've talked about this briefly with @HendrikThePendric and @cooper-joe and Joe suggested Number cannot be less than {lower} or more than {upper} (I'm paraphrasing because we were talking about a specific field). I think it'd be good to change the error message to what Joe suggested. It also doesn't make assumptions about either a float or an int, so seems ideal to me.

I think that adding e2e tests is very good and for a lot of things preferred over unit-tests. However, there are also some things that are more suited to unit testing. For example verifying that component props are mapped to element attributes correctly. This came up here dhis2/ui-core#630 (comment).

This issue serves as a reminder for us to start adding unit tests where appropriate.

Here is a suggestion for a position utility that can be used for popovers, tooltips and any other elements that are attached to an anchor.

12 available positions

The default positions would also include an offset value that defines how far from the anchor it should be placed. offset could be customised (depending on how complex this is to do). The offset in the example above is 4px.

Unsure about this: Position is also based on mouse cursor. So if an anchor is 500px wide, a bottom-right menu would appear 'bottom-right' in relation to where the mouse cursor is inside the anchor.

Smart fitting

If an element, a popover for example, does not fit on screen in it's defined position:

• Flip the orientation (top-left becomes bottom-left, right-bottom becomes left-bottom)

If said element still does not fit:

• place in original position and resize, vertical or horizontal, to fit on screen.

If element still does not fit and cannot be resized:

• place element offscreen. Layout/app has to handle this?

When a user doesn't have a profile image their initials are shown. (i.e. JT for John Traore). These initials are rendered in a <p> tag by the ui-widgets Headerbar, which seems semantically incorrect. It should probably be something neutral like <span> or something else that reflects its role on the page, but it is not a paragraph.

This can also easily lead to CSS leakage (inward) issues with alignment if the <p> tag is, for instance, assigned a margin globally (a common situation). See screenshot below, where a margin-bottom: 10px from a global CSS normalization causes misalignment in the Headerbar:

Since it's hard to predict how exactly our low-level components are going to be used, it could be beneficial to spread props onto them. Some relevant use-cases that are currently not supported, but that would be if we started spreading props:

• adding keyboard event listeners
• adding mouse event listeners
• adding aria attributes
• adding data attributes

Before implementing this we should first establish:

1. How other UI libraries deal with these type of things, perhaps spreading props isn't the only option to support the use-cases above
2. Whether the additional benefits outweigh the additional level of unpredictability

@HendrikThePendric, @ismay and I were talking about how we can check if some components follow a certain api so they'd work when used with another component. We've encountered some tricky scenarios which are hard to impossible to test (like testing if a component implemented a specific prop that we'll provide automatically; checking prop-types is not an option).

So the best way to cover this is by having proper docs which contain "How to create custom [Component name]" sections, which a developer most likely will read anyways when creating custom components.

Here's a list of components that require these docs:

• SingleSelect
  Options need to have a value and label, and accept an onClick prop
• MultiSelect
  Options need to have a value and label, and accept an onClick prop
• CheckboxGroup
  Checkboxes need either have or accept some specific props
• CheckboxGroupField
  Checkboxes need either have or accept some specific props
• RadioGroup
  Radios need either have or accept some specific props
• RadioGroupField
  Radios need either have or accept some specific props
• SwitchGroup
  Switches need either have or accept some specific props
• SwitchGroupField
  Switches need either have or accept some specific props

This new prop-type introduced in dhis2/prop-types#113 is more powerful/restrictive than just using propTypes.oneOfType, so we should use it where possible.

Steps:

• Do a global search in our codebase for propTypes.oneOfType
• Evaluate on a case-by-case basis if switching to the new prop-type would be appropriate

ui-core

• Snackbar (implemented as AlertBar)

ui-widgets

• Sidebar
• TreeView
• Tabs
• PeriodPicker*
• DatePicker*

* These are blocked by the multi-calendar date-engine that is going to power these components. This engine will be built by @abyot

Unsure

• Pagination

• Layout Components

  • Not needed in ui-core

• ListSelect Components

  • Not needed in ui-core
  • The searchable list component could be of interest

• Message

  • Could be used as the message for form input components as well

• MultiToggle

• SvgIcon

  • Wrapper for MUI Icons, which we don't want to use anymore
  • Should not be part of ui-core, could have its own lib (ui-icons?)

No.

• Heading
  Will be handled as part of the Typography implementation.

• ControlBar
  The control bar component was created specifically for the dashboard and has been removed from d2-ui: dhis2/d2-ui#431 so we do not need to port it.

Move to ui-core

We should move that component to ui-core for two reasons:

1. Other apps that do not rely on react-final-form should be able to use all our components. If they wanted to use the FileInput as it's implemented in ui-forms, they'd have to add ui-forms to their dependency list and use it with its to-RFF-adjusted props api
2. The intention of ui-forms is to be the bridge between our ui-libraries and react-final-form.
   Right now, the FileInput component is more than that. It's its own component that combines several building-blocks exposed from ui-core.

We can actually make it a controlled input

We can access and overwrite the .files property of the input DOM element, which means that every time the value prop changes, we can update the files attached to the DOM element.

That way the component that's currently in ui-forms would be controlled, just like all other input components in ui-core. It shouldn't be much of a problem to make it an uncontrolled component either once we've decided to go that way

Renaming it to FileInputWithList

Of course this is a component that makes use of ui-core's FileInput so it can't have the same name. But we also need to indicate that the behavior is slightly different, as choosing files after adding some beforehand will add the new files to the existing list instead of overwriting it. This needs to be taken into account when making the component a controlled component.

Summary

• Move component from ui-forms to ui-core
• Make component controllable
• Rename component (suggestion: FileInputWithList)

Relates to dhis2/ui-core#630

We reexport the react-final-form exports, but not the final-form exports. The final-form package contains exports that are also potentially useful in an app, like: import { FORM_ERROR } from 'final-form' (which I need atm.).

To unify the approach it'd be good to reexport final-form's exports as well. I'm not a fan of reexporting, but since we're doing it I think it'd be good to have a uniform approach. Addressing the above would allow apps to omit final-form from their dependencies (which technically they could already do, but it's common practice to list all packages you're importing from explicitly in package.json).

We talked a bit about the approaches people are using in the wild for managing paint order here: #10. There's an approach that Uber's taking that seems interesting to me, but it's difficult to know exactly how it'll work out for us without actually test-driving it a bit.

This issue is a reminder so the talks about our z-index strategy doesn't get lost, and also a reminder to me to create a small example implementation, so that we can see how it'd function IRL.

The Tab component has been given a style of outline: none, without any other alternative focus styling, which makes it really painful to navigate without a mouse. You should consider adding a outlining to it, as you have done with other components, such as the Button.

In dashboards, each dashboard item has a button that toggles the open/closed state of the item interpretations and description. I am trying to use the ui-core Secondary button for this, but it doesn't seem to currently support the 'active' state (which would indicate that the interpretations section is currently visible). Would it be possible to support this very soon (like this week?) 🙏

Tagging @cooper-joe in case he wants to comment on this.

Below is a mock up of what it would look like when active:

The button also gets a blue border due to the :focus state. I'm pretty sure this is not wanted for these buttons. If these requests are out of scope for the ui-core button, let me know soon and I'll create my own button for this.

I've updated the menu component with some extended functionality and cleaned up visuals.
The updated documentation
Direct link to updated design specs

The required changes, summarised:

• set up min/max width/height, detailed in spec notes
• adjust font size and padding for dense
• dense divider
• section headers (regular/dense)
• destructive type menu item
• disabled state menu item
• icon size standardization (regular/dense)

The design spec is much more detailed now, (the last one came straight out of MUI), the menu will now be more polished and extendable with the new features here 👍

Intro

The current version of the Menu and related components have a few issues that could be improved. Some changes were made in the past with a big focus on maintaining backwards compatibility (dhis2/ui-core#297 and dhis2/ui-core#354). We discussed on Slack that some more fundamental improvements couldn't be done without introducing breaking changes, and since we are now building up to a next major version we can now break 👹 and improve stuff. I'll go through the aspects of these components where there's room for improvement and conclude with a list of components I think we should implement for v5.

I had previously began to discuss various options for this with @Mohammer5 on Slack.

Making the menu click based

The current implementation responds to hover, and we discussed this needs to be changed to a click based implementation. For a click based menu we'd expect the following functionality:

1. Given that a menu has multiple sub-menus (at one level) and one is open, we'd expect that one to close when another sub-menu is opened (toggling). To achieve this we need to control which sub-menu is open from the parent menu.
2. When the user clicks outside of the menu, we'd expect all of the open sub-menus to close. Looking at the design specs, this behaviour doesn't have to be provided by the menu itself. Instead, the app would render a menu in a Popover and when the Popover's backdrop-layer is clicked would hide the Popover and the Menu as well.

Distinguishing between a menu-item and a sub-menu

Currently we only have a MenuItem component which has various props that determine the content of the menu-item, such as label and icon. And if the MenuItem has children then we assume it's a sub-menu. A dedicated SubMenu component would make things a lot more explicit, provide a clear separation of concerns, and would allow for the MenuItem to just render its children, which would be more in line with the rest of our components.

Separating behaviour from style

A fly-out menu has pretty opinionated behaviour. At the same time, a menu-list with menu-items could be a fairly generic way to present things. I've discussed this with @cooper-joe and he informed me that he presently knew of no other use-cases than outlined in the design-specs: a dropdown-style fly-out menu. But we both could see a plain menu-list with menu-items being used in a sidebar. So it would make sense to have a clear split between presentational and functional components, and this also fits very well with the distinctions between the packages in this mono-repo.

Dedicated components to reflect the design system

The design system specifically mentions dividers and section headers. We already have a generic Divider component which has the correct styling. For correct semantics we'd have to make sure that this is also wrapped in a li tag (given that the parent is an ul). And we'll definitely need to introduce a dedicated MenuSectionHeader component.

Allowing flexible menu-item content

One thing is for certain: the root element for a menu-item should be a li, because it is a direct descendant of an ul. However, wrapping the content in a a is perhaps not always what you would want to do, for example when using react-router, or when you are refactoring an app that is wrapping stuff in a tags already (?). One way to provide convenience and flexibility would be to use the pattern we established building the Tooltip. This would mean that the menu-item would render its children into an anchor by default, and if you want to opt-out of that, you can use the renderProps approach.

Suggested component set

Based on the summary above I've come up with a set of components. Quite likely, the details may change when I start implementing things, and when I receive input on this issue:

Presentational (i.e. ui-core or whatever name comes out on top):

• MenuList: an ul with appropriate styles that just renders children
• MenuListItem: a li with appropriate styles depending on props
• MenuSectionHeader: a li with perhaps a h6 and some specific styling
• MenuDivider: a li with a Divider

Behavioural:

• Menu: renders children into a Card with a MenuList and controls the toggling of the SubMenu open state
• MenuItem: by default this will render its children into a MenuListItem, but also supports renderProps for custom content.
• SubMenu: renders a MenuItem (I think via a menuItem prop. And conditionally renders a fly-out menu, which will be toggled on click.

I was working on the default translations for the FileInputFields and noticed that the placeholder for FileInputFieldWithList wasn't showing. The logic that decides whether a placeholder should show is in FileInputField.js:

                {!children && placeholder ? (
                    <FileListPlaceholder>
                        {translate(placeholder)}
                    </FileListPlaceholder>
                ) : (
                    children
                )}

The problem is that the FileInputFieldWithList passes this as children to the FileInputField:

                {files.map(file => (
                    <FileListItemWithRemove
                        key={file.name}
                        label={file.name}
                        removeText={translate(removeText)}
                        onRemove={this.handleRemove}
                        file={file}
                    />
                ))}

The default value for files is an array, so it'll always pass at least an empty array. So the children are never empty, and the placeholder will never show up. Unless I'm overlooking something. At any rate, the FileInputFieldWithList placeholder currently isn't showing in the story it seems.

The COVID user at https://covid.dhis2.org/demo/dhis-web-dashboard/ doesn't have access to Interpretations or Messaging apps, but the Headerbar still shows badges which link to those apps.

Interestingly, the user does have access to some interpretations, just not the app itself. I think in this case it's still the right move to hide the badge.