-
-
Notifications
You must be signed in to change notification settings - Fork 333
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
feat(step): circular steps variant #2917
Conversation
67a61f1
to
49e8a33
Compare
I had to fix stylelint errors and rebase to take out this commit, and done forced push. Would it be OK? |
Great! But why dont we just name it |
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 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.
I didn't implement for inverted yet. I don't know how the inverted should be at the moment. |
The connection line cover background could be avoided by .ui.railing.vertical.steps .step:not(:last-child) .content::before {
margin-top: 2.2em;
} it seems.... I wonder why i had so many issues doing that for connected feed... perhaps more difficult markup, Will take a look for that again |
That sound also OK for me. I will update the PR with that name and other fixes. |
Not much to be done i believe. I suggest to:
|
Nice! Would it be possible to have an option to have a title and/or description on the horizontal variant below the step circles? |
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 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.
The size variants need adjustments at least for vertical variant
tiny steps
massive steps
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 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.
The new variant seems to be always unstackable. This could be a design choice and perhaps supported later if a visually pleasing solution can be found
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.
my final comment/suggestion: 😉
disabled step
does not have any visual effect (you removed that on purpose...why?)
We should support color names like we have for the connected feed
variant.
Currently the green color is hardcoded (@completedColor
). The green was supposed to work for the usual step variant where coloring would not make so much sense to me as it would only have effect on the check icon.
But as your new variant concentrates on the icon and connection line there is a possible need for a different color (maybe only for a single completed step) and screams for being colorizable via a color class name.
Support coloring as we have it elsewhere would support
ui purple circular steps
blue completed step
teal step
- The connection line should inherit the color as we have it in
connected feed
I don't see any difference style for disabled step like box item. Maybe, remove the small circle disc? |
A disabled variant almost always has |
Disabled state would OK with completed or active. The default state already looks similar to the disabled item as it's color and background is grayed out. |
the opacity can be applied to either normal state or completed state. But you should still see the default (non-disabled, non completed) state greyed out (if no color is given as suggested above) and opacity:1 (it gets more clear when text is used in the vertical variant If you meant the same, then: yes 😄 |
49e8a33
to
77b6906
Compare
@lubber-de Here we go! I've finished the implementation with the other variantions and all your suggestions. Due to the nature and design approaches, the circular step doesn't have the following variations: stackable, unstackable, attached and evenly divided. I'm not sure what is going on with jsfiddle.net. It seems to be stopped working for me since yesterday. I cannot browse the website anymore. So, I've created the examples and demonstrations in CodeSandbox. |
Looking good 😊 The only bit I’m unsure about is the coloured line appearing after the active step, I think it should only go backwards to the previous step (so between completed and active) Possibly should have an option not to change the header colour as well on the horizontal version (or possibly if by adding Any possibility of having header supported below the horizontal version? Not sure if possible at all, but could even be pretty cool to have an optional pulsating active version (of the circle, could possibly have the line between active and last completed pulsating as well separately but certainly the active circle by itself) |
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.
👏🏼 This is already very awesome. Your example code is also a good candidate for later docs 😉
Please see my comments again
@jamessampford Yeah! That would be better I guess. What do you think @lubber-de ? |
Yes, agree |
Now, the color rule for the rail line is changed as suggested by @jamessampford. The line only after the completed step is now showing the particular color. The rest of the states would use the default color for the rail line. Jsfiddle: |
I’ve managed to add optional pulsating to active - https://codesandbox.io/s/fui-circular-step-forked-7tqhs6?file=/index.html - probably needs some tweaking … borrowed from transitions, though needs to target the |
The new variant circular step shows a series of steps with the circular marker and connection line between them. It supports the three layout styles: horizontal, vertical and ordered. It has the same states as box step: active, completed, disabled. And it contains the variations: color, inverted, size. Due to it's design nature, it doesn't have the variations such as: stackble, unstackable, attached and evenly divided.
Only the rail line after the completed step has particular color. The other states use the default color for the rail line.
27877fb
to
89dc439
Compare
@jamessampford You can submit your own PR later for any improvements. I would like to keep it as simple for now. |
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.
That's very awesome by now!
Just a little comment (see below) and a general question (not a request):
There are now a very lot of :not(.circular)
selector enhancements.
Would it be easier (less code?) to instead override the circular setting (which would be needed for non circular only) instead directly. This is especially noticable for all the evenly divided variants (.x.steps)
I understand you did that because circular steps are styled differently, but perhaps it's worth some minutes to think about that again.
So instead of
.ui.steps:not(.circular) {
display: inline-flex;
}
just override it for the ciscular variant like
.ui.steps {
display: inline-flex;
}
.ui.circular.steps {
display: block;
}
I'll try to review again and find out whether it's possible to avoid excluding with |
I tried to remove the |
Guys this project still good? |
I added support for horizontal content. |
Description
This PR add the new variant
railing
to the steps which suppose to display the steps with circular railing horizontally or vertically.I used the name
raliing
since we already haverail
component which can conflict each other. If you have better name for this variant, please suggest.Testcase
https://jsfiddle.net/ko2in/n9dum7fg/
Screenshot (if possible)
Closes
#2170