-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
gabbsmo
wants to merge
5
commits into
n8design:main
Choose a base branch
from
gabbsmo:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,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> | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
--- | ||
order: 30 | ||
--- |
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
@forward "progress"; | ||
@forward "shimmer"; | ||
@forward "spinner"; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
/* | ||
* Animation from Fluent React | ||
* MIT license: https://github.com/microsoft/fluentui/blob/master/packages/react/LICENSE | ||
*/ | ||
@keyframes hoo-spinner { | ||
0% { | ||
transform: rotate(0deg); | ||
} | ||
|
||
100% { | ||
transform: rotate(360deg); | ||
} | ||
} | ||
|
||
%hoo-spinner { | ||
display: block; | ||
box-sizing: border-box; | ||
border-radius: 50%; | ||
border-width: 1.5px; | ||
gabbsmo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
border-style: solid; | ||
border-color: var(--themePrimary) var(--themeLight) var(--themeLight); | ||
border-image: initial; | ||
animation-name: hoo-spinner; | ||
animation-duration: 1.3s; | ||
animation-iteration-count: infinite; | ||
animation-timing-function: cubic-bezier(0.53, 0.21, 0.29, 0.67); | ||
} | ||
|
||
.hoo-spinner-medium { | ||
@extend %hoo-spinner; | ||
width: 1.25rem; | ||
height: 1.25rem; | ||
} | ||
|
||
.hoo-spinner-xsmall { | ||
@extend %hoo-spinner; | ||
width: 0.75rem; | ||
height: 0.75rem; | ||
} | ||
|
||
.hoo-spinner-small { | ||
@extend %hoo-spinner; | ||
width: 1rem; | ||
height: 1rem; | ||
} | ||
|
||
.hoo-spinner-large { | ||
@extend %hoo-spinner; | ||
width: 1.75rem; | ||
height: 1.75rem; | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
So default spinner with modifier classes.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all great work so far.
Shouldn't those already be scope to the outer classes? Cannot recal that we had a medium in there that is not scoped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a non scoped
.medium
selector in pattern-scaffolding.scss.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not anymore and it was not part of htwoo anyways. scoped it now to body.
Check out the core-v2.5.0 branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I pushed changes that use modifier classes instead of one class per size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you will check it out and merge it for the version.