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

Spinner implementation #120 #135

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Spinner implementation #120 #135

wants to merge 3 commits into from

Conversation

gabbsmo
Copy link

@gabbsmo gabbsmo commented May 16, 2024

Spinner implementation based on styles from Fluent React, but adopted to fit into htwoo's convention.

htwoo-core/src/styles/01-atoms/loading/_spinner.scss Outdated Show resolved Hide resolved
Comment on lines 1 to 4
<div class="hoo-spinner-xsmall"></div>
<div class="hoo-spinner-small"></div>
<div class="hoo-spinner-medium"></div>
<div class="hoo-spinner-large"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't put all the spinner in just one file. This avoid's to get reused - Instead use variations of the spinner like:
https://patternlab.io/docs/using-pseudo-patterns/

Also the default verison is missing.

Copy link
Author

Choose a reason for hiding this comment

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

Gave it a try. I am completely new to Pattern Lab.

Default and Medium is equivalent. Can remove one of them, whichever you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just show the default one. Add a markdown that it equals the medium spinner.

One thing we could do in case of the spinner have something like this:

.hoo-spinner{
    
    &.xsmall{
    }

    &.small{
    }
    
     &.large{
    }
    
}

So default spinner with modifier classes.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I think I tried modifier classes at first but found that .medium collided with some other already existing style. Because of that I went with the appoarch in the PR. It also seemed to be more in line with other styles such as .hoo-button, .hoo-button-primary, rather than .hoo-button.primary.

But if you prefer the style in your example we can go with that of course.

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.

None yet

2 participants