Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: move all core components to widgets #5

Merged
merged 6 commits into from
Mar 24, 2020

Conversation

varl
Copy link
Contributor

@varl varl commented Mar 23, 2020

This introduces the new separation of what is a core component (style +
structure) and what is a widget (behavior). The idea is to keep core
components very stable and generic over time, and instead using the
concept of "widges" to mean things that are fully formed and usable
directly in an application.

This stability means that it becomes easier and safer to build new
components by composing core components. It also provides a clear
distinction between what is a core component and what is a widget.

BREAKING CHANGE: All @dhis2/ui-core exports have been migrated to
@dhis2/ui-widgets.

This introduces the new separation of what is a core component (style +
structure) and what is a widget (behavior). The idea is to keep core
components very stable and generic over time, and instead using the
concept of "widges" to mean things that are fully formed and usable
directly in an application.

This stability means that it becomes easier and safer to build new
components by composing core components. It also provides a clear
distinction between what is a core component and what is a widget.

BREAKING CHANGE: All @dhis2/ui-core exports have been migrated to
@dhis2/ui-widgets.
@varl varl requested a review from a team as a code owner March 23, 2020 16:39
Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

I'm probably a bit early for a proper review, but left some comments anyway.

packages/core/src/ButtonBase/ButtonBase.js Outdated Show resolved Hide resolved
Comment on lines +5 to +31
export default css`
.icon {
padding-left: ${spacers.dp12};
}

.icon-only.icon {
padding-left: 6px;
}

.icon-only {
padding: 0;
}

.icon-only i {
margin-right: 0;
margin-left: 0;
}

.button-icon {
margin-right: 6px;
color: inherit;
fill: inherit;
font-size: 26px;
vertical-align: middle;
pointer-events: none;
}
`
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning behind this split? I'd expect that the icon-related styles are also just part of the ButtonBase. I'm sure you've got a good reason for it, but I don't immediately see it....

@varl
Copy link
Contributor Author

varl commented Mar 24, 2020

@HendrikThePendric If you would like to review this, go ahead. Keep this in mind:

  • This moves all our current components to widgets as they are.
  • One component has then been extracted to core (ButtonBase) from widgets (Button).
  • The ButtonBase is only style + structure and is intentionally kept very generic in terms of props.
  • Base components need to be exported with a forwardRef so they can be kept very generic in terms of behavior and allow widgets to build on them.
  • This PR does not intend to extract all of the base components to core, that will be done component by component so more people can contribute.

The main thing to think about:

  • core: base components that consist of style + structure
  • widgets: components that provide behavior and are ready to be used in a UI

@HendrikThePendric
Copy link
Contributor

@varl thanks for explaining, luckily that's how interpreted the changes I was seeing already 😄

The ButtonBase is only style + structure and is intentionally kept very generic in terms of props.

Is the comment above related to my #5 (comment)? I still don't really see why the icon related logic cannot be classified as "style and structure". Could you elaborate?

@varl
Copy link
Contributor Author

varl commented Mar 24, 2020

@varl thanks for explaining, luckily that's how interpreted the changes I was seeing already smile

The ButtonBase is only style + structure and is intentionally kept very generic in terms of props.

Is the comment above related to my #5 (comment)? I still don't really see why the icon related logic cannot be classified as "style and structure". Could you elaborate?

The icon is a component itself, so it has its own "style and structure" that is not part of the ButtonBase atom.

Having an "icon button" is a DHIS2 specific behavior that composes a ButtonBase and an Icon, so that's why it belongs in Button.

@HendrikThePendric
Copy link
Contributor

The icon is a component itself, so it has its own "style and structure" that is not part of the ButtonBase atom.

Having an "icon button" is a DHIS2 specific behavior that composes a ButtonBase and an Icon, so that's why it belongs in Button.

Aha. Yes, the penny has dropped. Thanks for that.

@varl varl merged commit d671d09 into alpha Mar 24, 2020
@varl varl deleted the refactor/structure-style branch March 24, 2020 10:48
@dhis2-bot
Copy link
Contributor

@dhis2-bot
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants