dhis2 / ui Goto Github PK
View Code? Open in Web Editor NEWComponents and related resources for the DHIS2 design system
Home Page: https://ui.dhis2.nu
License: BSD 3-Clause "New" or "Revised" License
Components and related resources for the DHIS2 design system
Home Page: https://ui.dhis2.nu
License: BSD 3-Clause "New" or "Revised" License
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:
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.
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.
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:
yarn lint
the rule was applied tooyarn 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.
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 runError [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.
Popper dropdown for select positioned incorrectly after resize
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.
Headerbar should hide Interpretations and Message badges when user doesn't have access to those apps
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.
Update validation error for the createNumberRange validator
Currently the validation error for the
createNumberRange
validator displays'Please enter a number between {{lowerBound}} and {{upperBound}}'
, but the validation allowslowerBound
andupperBound
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.Misaligned arrow icon in nodes
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 theNode
.Default translations for widgets
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
Components should be able to accept a ref
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.Menu: update/extend from specs
I've updated the
menu
component with some extended functionality and cleaned up visuals.
The updated documentation
Direct link to updated design specsThe 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 π
Transfer component - infinite scrolling
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
Display submit errors as well in our field components
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).Fix `Cannot update a component from inside the function body of a different component` warning
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
Reexport final-form from ui-forms as well
We reexport the
react-final-form
exports, but not thefinal-form
exports. Thefinal-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 omitfinal-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).Switch to react-popper for our Popper
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 ourPopper
.The automated release is failing π¨
π¨ 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:
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 URLhttps://[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 π¦π
Implement `conditional` prop-type where appropriate
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
Use less (or simpler) test helpers
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.Clarify the assumptions made in our features/step definitions/stories
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.
className prop targets child element instead of top level parent
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:
ToggleGroup
/RadioGroup
- applies to the children, leaving an unstylablediv
at the top level.Add `onFocus` and `onBlur` to Button
When I was implementing the
FileInput
inui-forms
I realised that currently it's not possible to attachonBlur
andonFocus
to theui-core
FileInput
. The reason for this is that the only visible UI element is a button (i.e. ourButton
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
andonBlur
prop to theButton
instead.Revisit Select
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.
Issues with context when using different instances of our libs
@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 theanalytics
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
Reorganise overlay components
Currently we have several components with overlapping responsibility and some duplication. Some things I noticed:
ScreenCover
uses aBackdrop
, but itβs a private one, not the one that we expose..- Suppose the (public)
Backdrop
is meant to be placed into aScreenCover
orComponentCover
(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 aContent
component, which vertically and horizontally centers its content. Again, I think this is too opinionated.- The
ComponentCover
does something similarSo 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 anonClick
, has a semi-transparent background by default and atransparent
prop. This component just takes up 100% width and height of the parent so it can be used to make aLayer
orComponentCover
"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
Discussion: Position utility
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). Theoffset
in the example above is4px
.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?
Missing components in ui-core vs. d2-ui-core
ui-core
- Snackbar (implemented as AlertBar)
ui-widgets
* These are blocked by the multi-calendar date-engine that is going to power these components. This engine will be built by @abyot
Unsure
- Not needed in
ui-core
- Not needed in
ui-core
- The searchable list component could be of interest
- Could be used as the message for form input components as well
- 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 FileInput from ui-forms to ui-core
Move to ui-core
We should move that component to
ui-core
for two reasons:
- 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 inui-forms
, they'd have to addui-forms
to their dependency list and use it with its to-RFF-adjusted props api- The intention of
ui-forms
is to be the bridge between our ui-libraries and react-final-form.
Right now, theFileInput
component is more than that. It's its own component that combines several building-blocks exposed fromui-core
.We can actually make it a controlled input
We can access and overwrite the
.files
property of theinput
DOM element, which means that every time thevalue
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 inui-core
. It shouldn't be much of a problem to make it an uncontrolled component either once we've decided to go that wayRenaming it to
FileInputWithList
Of course this is a component that makes use of
ui-core
'sFileInput
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
toui-core
- Make component controllable
- Rename component (suggestion:
FileInputWithList
)
Relates to dhis2/ui-core#630
Scope for ui@5
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.
Uniform organisation for our feature files / step definitions in cypress/integration
We currently have e2e stories defined in core, widgets and forms. In
cypress/integration
we have aforms
folder that contains the feature files and step definitions for theforms
stories. But forcore
andwidgets
those are all mixed at the root ofcypress/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
Add active state for 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.Rerunning cypress tests (GH actions) breaks
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
Document required props on children for parent components that rely on them
@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 avalue
andlabel
, and accept anonClick
prop- MultiSelect
Options need to have avalue
andlabel
, and accept anonClick
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 propsUse .should instead of .then to retry assertions in cypress
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.
Create example implementation of uber-based layering approach
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.
Dependabot couldn't find the branch master
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.Spreading props
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:
- How other UI libraries deal with these type of things, perhaps spreading props isn't the only option to support the use-cases above
- Whether the additional benefits outweigh the additional level of unpredictability
SplitButton in ButtonStrip missing margin
Statusicon rendering causing alignment issues
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
Move unit tests to jest instead of cypress
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.
ComponentCover doesn't cover parent component
The
<ComponentCover />
component has css withwidth: inherit
andheight: inherit
. This works well if the parent of the cover has explicitwidth
andheight
set, bit it breaks when the parent is dynamically sized - for instance, if the parent is the child of aflex
container.Is there reason
inherit
was chosen and could we perhaps support another mechanism to cover dynamically-sized parent elements? In many caseswidth: 100%
,height: 100%
may work, but that could have problems determining the proper height.Post-Transfer merge improvements
The parts I noticed:
We are moving away from
{ value, label }
objects for selected values and instead using string values.The multiselect proptypes is gone, there's a
TODO
where I've copied in the old shared prop types for multiselect, see point 1.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.)
I'm not fond of the
common
pieces. I had a hard time keeping track if I was looking atTransfer/__e2e__/common.js
,__tests__/common/*
, orTransfer/common.js
.Please specify the extension when importing (this is the code style in UI !). Using
import { foo } from 'directory'
orimport { foo } from 'file'
to import fromdirectory/index.js
orfile.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.The
__tests__/common
has several options:
- Add a private package to
packages/test-utils
, then we can importcreateChildren
fromui-test-utils
(I like this one).- Create a folder in
packages/core/src
namedutils
and putcreateChildren.js
there.- Don't automate this.
Originally posted by @varl in #49 (comment)
Left aligned labels for composed 'Field' elements
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 defaultselect
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 theselect
. I assume this would simplify the build of theselect
field too, and we avoid the awkward question you raised: why isn't 'inline label' available forinput
too?Menu refactor
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:
- 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.
- 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 aslabel
andicon
. And if theMenuItem
haschildren
then we assume it's a sub-menu. A dedicatedSubMenu
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 ali
tag (given that the parent is anul
). And we'll definitely need to introduce a dedicatedMenuSectionHeader
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 anul
. However, wrapping the content in aa
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 ina
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
: anul
with appropriate styles that just renders childrenMenuListItem
: ali
with appropriate styles depending on propsMenuSectionHeader
: ali
with perhaps ah6
and some specific stylingMenuDivider
: ali
with aDivider
Behavioural:
Menu
: renders children into aCard
with aMenuList
and controls the toggling of theSubMenu
open stateMenuItem
: by default this will render its children into aMenuListItem
, but also supports renderProps for custom content.SubMenu
: renders aMenuItem
(I think via amenuItem
prop. And conditionally renders a fly-out menu, which will be toggled on click.ui@5 alpha - Questions
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 descriptionResolved questions
none yet
The Tab component lacks a11y
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.Add e2e tests for new popper & popover features
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.
Transfer component - loading spinner
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
Layer stacking
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 theLayer
component can calculate a newz-index
and return a newProvider
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 theLayerContext
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.
Recommend Projects
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
TensorFlow
An Open Source Machine Learning Framework for Everyone
Django
The Web framework for perfectionists with deadlines.
Laravel
A PHP framework for web artisans
D3
Bring data to life with SVG, Canvas and HTML. πππ
Recommend Topics
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
web
Some thing interesting about web. New door for the world.
server
A server is a program made to process requests and deliver data to clients.
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Visualization
Some thing interesting about visualization, use data art
Game
Some thing interesting about game, make everyone happy.
Recommend Org
We are working to build community through open source technology. NB: members must have two-factor auth.
Microsoft
Open source projects and samples from Microsoft.
Google β€οΈ Open Source for everyone.
Alibaba
Alibaba Open Source for everyone
D3
Data-Driven Documents codes.
Tencent
China tencent open source team.