Skip to content

refactor: monorepo - #2

Merged
varl merged 93 commits into
alphafrom
monorepo
Mar 19, 2020
Merged

refactor: monorepo#2
varl merged 93 commits into
alphafrom
monorepo

Conversation

@varl

@varl varl commented Mar 6, 2020

Copy link
Copy Markdown
Contributor

I recommend cloning this branch and exploring locally.

This supersedes dhis2/ui-core#822. This approach uses Git subtrees to enable us to pull in changes that are happening in ui-core and ui-widgets without having to rebase everything.

To do

  • Pull in @dhis2/ui-core
  • Pull in @dhis2/ui-widgets
  • Pull in @dhis2/ui-forms
  • Extract @dhis2/ui-icons (holds all the icons used in UI)
  • Extract @dhis2/ui-constants (holds the constants, e.g. theme, spacers, elevation, etc) that are reused in UI
  • Create @dhis2/ui which is a metapackage that enables a consistent import path for everything
  • Colocate as much code related to a component as is possible (demo/e2e stories, styles, unit tests, etc.)
  • Migrate components from core that should be in widgets
  • Restructure Storybook
  • Rebase and reword non-conventional commits

varl added 17 commits March 6, 2020 16:38
The commits from the original branch were lost, so here are the
breakings changes listed to make sure they show up in the changelog.

BREAKING CHANGE: Rename the Constrictor component to Box, which is
shorter and thus easier to type. This also expands the capabilities of
Box to make it more Box-like.

BREAKING CHANGE: Replace SwitchGroupField, RadioGroupField,
CheckboxGroupField with ToggleGroupField.

BREAKING CHANGE: Replace SwitchGroup, RadioGroup, CheckboxGroup with
ToggleGroup.

BREAKING CHANGE: The exports: colors, theme, layers, spacers,
spacersNum, and elevations, have been moved from @dhis2/ui-core to
@dhis2/ui-constants for better code reuse.
@Mohammer5

Copy link
Copy Markdown
Contributor

I think it'd be good to give the icons unique names, they don't need to be easily "human" readable, as long as they're unique (e. g. D2ICON1, D2ICON2, etc).

This way we can add as many "arrow" as we want and re-use very specific types in our components, just like we re-use the colors.500 as theme.error color (red500 -> error -> Field uses error for displaying an error). Then we can easily switch out icons in the intermediate stage while not having to repeat ourselves (otherwise an icon library is kind of senseless?): D2ICON1->core/Node/Arrow->core/Node. Changing the arrow of the Node won't impact any other component and D2ICON1` can safely be re-used, as long as we never modify but deprecate existing icons and add new ones if we want to entirely replace an icon.

But that's something we'd have to discuss with @cooper-joe as well

@varl

varl commented Mar 12, 2020

Copy link
Copy Markdown
Contributor Author

@Mohammer5 Interesting. Icons have been requested to be externally available too, e.g. from Apps.

Does that change anything or do you still think the non-human readable icon names is the way to go?

@Mohammer5

Copy link
Copy Markdown
Contributor

Does that change anything or do you still think the non-human readable icon names is the way to go?

I don't really have an opinion on this. I think for us it doesn't really matter (a descriptive name could be easier to others though). Maybe it's generally better to have descriptive names (like D2-Icon-Arrow-1 for example).

I'm used to generic names so people don't interpret too much, but rather look at the styleguide for when to use which icon (i. e. when looking for an arrow pointing right and there are several available but the styleguide has rules for them as they should only be used in a certain context)

@varl

varl commented Mar 12, 2020

Copy link
Copy Markdown
Contributor Author

rather look at the styleguide for when to use which icon

Good point, we have "cryptic naming schemes" for spacers (dp4, dp24, ...) and colors (blue900, gray050, ...) so that the style guide references, so keeping icons inline with that seems like a good option to keep it unambiguous.

@Mohammer5

Mohammer5 commented Mar 12, 2020

Copy link
Copy Markdown
Contributor

Right now, this will expose our - as of now - internal shared prop types. Is that intentional?

@varl

varl commented Mar 12, 2020

Copy link
Copy Markdown
Contributor Author

Right now, this will expose our - as of now - internal shared prop types. Is that intentional?

Yes. Not through the @dhis2/ui package (which will be the main entry-point for most users), but it will be published separately.

@Mohammer5

Copy link
Copy Markdown
Contributor

Yes. Not through the @dhis2/ui package (which will be the main entry-point for most users), but it will be published separately.

Maybe it'd then makes sense to bundle them with @dhis2/prop-types (we could move that one into the mono repo as well?)


I'm not sure we should expose them though as keeping them internal allows us to change them without "breaking" changes as they could be used elsewhere (and totally out of context) when exposed

@varl

varl commented Mar 12, 2020

Copy link
Copy Markdown
Contributor Author

Yes. Not through the @dhis2/ui package (which will be the main entry-point for most users), but it will be published separately.

Maybe it'd then makes sense to bundle them with @dhis2/prop-types (we could move that one into the mono repo as well?)

Hmm. The reason they are in @dhis2/ui-constants is because a component that e.g. wraps a Button will need to comply with the same prop types as we use internally. Either they manually construct them, or we expose our abstraction.

I don't know if we want to make those a part of the @dhis2/prop-types library. Feels like mixing concerns, but I haven't though much about it so it's something we can look into.

@varl

varl commented Mar 12, 2020

Copy link
Copy Markdown
Contributor Author

I'm not sure we should expose them though as keeping them internal allows us to change them without "breaking" changes as they could be used elsewhere (and totally out of context) when exposed

If we change a components prop-types, that changes the API, and is a breaking change anyway. Or am I missing something here?

@Mohammer5

Copy link
Copy Markdown
Contributor

I don't know if we want to make those a part of the @dhis2/prop-types library. Feels like mixing concerns, but I haven't though much about it so it's something we can look into.

I agree. My personal preference would be to keep them internal until request to expose them come in.


If we change a components prop-types, that changes the API, and is a breaking change anyway. Or am I missing something here?

Yeah, if they're misused. Let's say we add a status to the statusPropTypes which is not handled by components that have nothing to do with our status components because the developer thought that this will cover all our statuses. If we want to add a status, this could be a potential breaking change (while we're just adding a feature)

@varl

varl commented Mar 12, 2020

Copy link
Copy Markdown
Contributor Author

OK. To be frank I've disliked the common-prop-types.js file from day 1, I'd rather just have the code duplicated in each component. The same problem you described applies to the internal common-prop-types.js as well.

Adding a new status to the internal shared prop-types will still cause it to be applied everywhere that shared prop-type definition is used, and thus may cause breaking changes.

It would just be limited to our library and not external devs using it.

@varl
varl changed the base branch from master to alpha March 19, 2020 10:43
@varl
varl merged commit 2a9b7a2 into alpha Mar 19, 2020
@varl
varl deleted the monorepo branch March 19, 2020 10:44
@dhis2-bot

Copy link
Copy Markdown
Contributor

@dhis2-bot

Copy link
Copy Markdown
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