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

Update workflow #21

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Update workflow #21

wants to merge 22 commits into from

Conversation

S-unya
Copy link
Member

@S-unya S-unya commented Nov 18, 2022

  • Update workflow
  • Add example Radix component
  • Add example bespoke component
  • Setup storybooks
  • Setup vitest

README.md Show resolved Hide resolved
README.md Outdated
@@ -30,7 +30,7 @@ import {
FormInput,
FormInputGroup,
FormSelect
} from 'aerian-component-library';
} from 'aerated';

Choose a reason for hiding this comment

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

... is it just aerated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure yet... I will work this all out once we decide on the way that we are managing versioning,etc. I'm going to remove all this.

return "props" in element;
};

export interface AerAlertDialogProps extends DefaultProps<"div"> {

Choose a reason for hiding this comment

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

I'm not sold on the API of this atm if I understand it correctly - I'd need to import three components to have it working correctly. What are the benefits of requiring this over providing more props for text elements etc.
And some other thoughts:

  • trigger can just be a string?
  • Can dialogTitle type include string and render a default h tag? Though the question would be what's the appropriate h tag? If the wrapper is a section, does it matter? 🤔
  • I'd prefer props as foo over dialogFoo - it's already being called as a dialog

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. The purpose of the multiple elements imports was to give more control over the styling of, e.g. the footer by passing className. Given that I have removed the "trigger" as it wasn't giving that benefit, but I can still see the need for a footer that you can pass a className to.
  2. Trigger must be a button, and I have now typed it as such
  3. I have allowed the title to be a string now - this is a good suggestion for DX 👍
  4. Agreed, done.

) : (
<AlertDialog.Title
className={cx(styles.title, {
[styles.visuallyHidden]: dialogTitle.hideTitle === false,

Choose a reason for hiding this comment

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

should this be the inverse?

Suggested change
[styles.visuallyHidden]: dialogTitle.hideTitle === false,
[styles.visuallyHidden]: dialogTitle.hideTitle,

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

done

import { DefaultProps } from "../../types/types";
import styles from "./AerButton.module.scss";

export type AerButtonVariants =

Choose a reason for hiding this comment

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

No secondary?!

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a default and a primary, tertiary is deliberately skipping a level to indicate it is much less obvious than the other buttons... though it might be better named linkLike... what do you think?

Comment on lines +9 to +12
| "primary"
| "tertiary"
| "important"
| "default";

Choose a reason for hiding this comment

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

you could make these a discriminated union of booleans, allowing only one at a time, like this
it's pretty verbose, but makes for a nice api

Copy link
Member Author

Choose a reason for hiding this comment

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

That is useful for elsewhere, but you cannot put 2 different versions of this in the variant... so perhaps overkill for this?

line-height: var(--lh-heading-xs);
}

strong {

Choose a reason for hiding this comment

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

Maybe we could also add styles for <em> and <s> if relevant?

Copy link
Member Author

Choose a reason for hiding this comment

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

They will have browser styles, the purpose of this is so that it picks up the correct font.

@DanCoderDev
Copy link

From what I could understand, all looked good to me. Just a couple of CSS changes to consider:

  • Could add a CSS/SCSS file for backgrounds -> In here, we could set colour contrast functionality (e.g. if background colour is black, the text is white, etc.)
  • Could add a CSS/SCSS file for accessibility -> Have a central place for everything accessibility (e.g. could set reduced motion styles, etc.)

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

Successfully merging this pull request may close these issues.

3 participants