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

Big refactor: all things working #56

Merged
merged 12 commits into from
Sep 10, 2024
Merged

Big refactor: all things working #56

merged 12 commits into from
Sep 10, 2024

Conversation

emi420
Copy link
Collaborator

@emi420 emi420 commented Sep 9, 2024

This PR has all things working and serves as a starting point for the development of the library.

Styling

This was one of the main issues, some styles were working on certain contexts (ex: web components Storybook) and failing in others (ex: React example). The main issue was with the styles provided by UnoCSS, utility classes like "flex" or "text-primary" were not always working. I've tried with Tailwind CSS but the issue persisted. The solution was to write the styles in CSS, this is something trivial as the utility classes are really basic.

Now you'll find that there's a style file for each component, for exampleheader.styles.ts.

We still use Class Variance Authority and make heavy use of CSS global variables, the ones provided by Shoelace prefixed with --sl- and some specific things prefixed with --hot-.

Variables are defined (or redefined for Shoelace) in two files: theme/hot.css and theme/hot-sl.css. I decided to use a single CSS file for color variables with the re-definition of Shoelace values because this library depends on it and it was not convenient to define variables twice.

This solves #55 #53 #51 #50 #33 #34

Logo component

This new component displays the HOT english logo (image and text) as default, using SVG files.

This solves #45

Storybook for React

It's really convenient for testing components in React while doing development, ensuring compatibility. In the future we could integrate both Storybooks, having an option for HTML and React, like the Shoelace docs.

For starting the React Storybook: pnpm run dev-react

This solves #54

Updated app examples

All examples were updated, not only in the app code but also the library dependency, using git+https://github.com/hotosm/ui.git#dev instead of a link. This is to make sure that the library is working in a separated app. You can even copy any example to a path outside the library, run pnpm install && pnpm run dev and it will work.

Shoelace components

Shoelace web components are now exported without the Sl prefix, so you can import them like this:

import { Toolbar, Button, Header, Logo } from '@hotosm/ui/components';

Same with the React ones:

import { Toolbar, Button, Header, Logo } from '@hotosm/ui/components/react';

This makes things more consistent.

Updated documentation

The Readme.md file was updated to be more clear and concise, some of its content will be moved to a detailed documentation later.

@emi420
Copy link
Collaborator Author

emi420 commented Sep 9, 2024

What we need to be working is styling and functionality, for both HOT custom components and Shoelace wrappers, not only when linking the package but also when installing it from command line using the repository URL (until we publish the package to the NPM registry), like this:

pnpm install "git+https://github.com/hotosm/ui.git#dev

I had to remove some dependencies and use a different strategy, specially for styling, because using neither UnoCSS nor Tailwind was not working when trying to install the package. The current approach is working, I've tested this branch in all the different contexts (Storybook, React Storybook, React, React 19, Svelte, Vue and HTML) and it's working on all of them.

Docs should be updated and some styles needs to be fixed before merging and continue working on components.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Sep 10, 2024
@emi420 emi420 marked this pull request as ready for review September 10, 2024 19:00
Copy link
Collaborator

@JoltCode JoltCode left a comment

Choose a reason for hiding this comment

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

Awesome work! Lots of great stuff in this PR 🚀. LGTM - I think we just need to look over a few of the points in my comments.

@@ -0,0 +1,16 @@
// @ts-ignore
Copy link
Collaborator

@JoltCode JoltCode Sep 10, 2024

Choose a reason for hiding this comment

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

Why are we ts-ignore'ing quite a few of the react files?

Copy link
Member

@spwoodcock spwoodcock Sep 10, 2024

Choose a reason for hiding this comment

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

I actually believe we could delete the react components.

It's extra work maintaining & this is a new lib for new tools. We can just require minimum React 19.

FMTM and DroneTM are on 19.
The fAIr UI rewrite will also be React 19.
Those are the first users of this lib.
(fAIr rewrite was decided recently)

What do you think?

Copy link
Collaborator Author

@emi420 emi420 Sep 10, 2024

Choose a reason for hiding this comment

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

Oh, that line was there before and I forgot to check why it was there.

I don't think we should delete the React components, even Shoelace has React support. Now that we have everything working you just need to add a wrapper with createComponent and an export in components/react/index.js.

Copy link
Member

@spwoodcock spwoodcock left a comment

Choose a reason for hiding this comment

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

Love it! Great work 🙌

As Joe said, we can possibly refine some extra things together in the future, but the main thing is right now that this works nicely and we can start designing new components 😃


🖥️ **Source Code**: <a href="https://github.com/hotosm/ui" target="_blank">https://github.com/hotosm/ui</a>
HOT themed UI components to reduce code duplication and make live easier for developers, available as [Web Components](https://developer.mozilla.org/en-US/docs/Web/API/Web_components) and witn first-class React support
Copy link
Member

Choose a reason for hiding this comment

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

available as [Web Components](...), with support for usage in [almost all](https://custom-elements-everywhere.com) modern web frameworks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But HOT UI is also working in React 18

# HOT Shared UI

<!-- markdownlint-disable -->
<p align="center">
Copy link
Member

Choose a reason for hiding this comment

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

I preferred this centralised layout, as it aligns with READMEs in our other tools too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes? it's not very clean ..

Copy link
Member

Choose a reason for hiding this comment

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

Clean based on what metric?

Sure it's HTML in Markdown which isn't ideal, but it makes for a nicer looking README file so it's worth it.

So many other projects use this approach 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean, I don't think the design and content organization is clean, using HTML is ok. IMO it should be easier to understand, with more consistent alignment and without excessive amount of information.

@spwoodcock
Copy link
Member

spwoodcock commented Sep 10, 2024

Should we remove the React components as they are unnecessary overhead?

Plus the index import xxx as xxx can be looked at as Joe commented.

Otherwise, we are set to merge 👍

@emi420 emi420 self-assigned this Sep 10, 2024
@emi420 emi420 merged commit aaba2a2 into main Sep 10, 2024
1 of 2 checks passed
@spwoodcock spwoodcock deleted the dev branch September 11, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants