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

[Toast] Progressbar, closeEasing, Bugfixing, message support #139

Merged
merged 13 commits into from
Oct 10, 2018

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Sep 28, 2018

Toast Enhancements

Added

  • Option showProgress (default false), which displays a SUI attached progressbar increasing until displayTime is reached (ignored, if displayTime = 0) Values: 'bottom','top',false
  • Option progressUp (default true). If set to false the progress is decreasing instead of increasing
  • Support for 'message' component in addition to 'toast' to work with progressbar if className: {toast: 'ui message'} is used (all message color options are supported to display a progressbar with the same color)
  • Option transition.closeEasing (default 'easeOutBounce') to animate the stacking when a toast closes. Set empty for old behaviour
  • Optional onBeforeHide (will be called given a callback parameter) functionality to transition.js to make the above closeEasing animation possible
  • Support className: {toast:'ui floating toast'} just like in message to display a box-shadow underneath/around the toast. (className: {toast:'ui hoverfloating toast'} will show the box-shadow only on hovering)
  • Option closeIcon (default false). This will make the toast closable by the top right corner icon instead of clicking anywhere on the toast
  • Support string to option showIcon to be able to use individual icons

Changed

  • Removed fixed width, added an option compact (default true) which does the same but does this independ of the className
  • Removed fixed Opacity, added an option opacity (default 1) instead as discussed below

Fixed

  • Timeout bug: If toast was closed by click, the timeout was still running and returned error when reached because $toast was not existing anymore
  • Icon positioning on large wrapping text in toast to stay at the top left of the text
  • Icon will only be rendered, if class exists in icon object
  • Alignment of toast with different widths in right and center position

Examples

Progressbar showProgress: 'bottom'

toast_progressbar

$('body')
          .toast({
            displayTime: 5000,
            message: 'I am a toast, nice to meet you! this is a lot of text and should be displayed properly',
            class: 'info',
            showIcon: false,
            showProgress: 'bottom'
        });

Progressbar decreasing progressUp: false

toast_progressup_false

$('body')
          .toast({
            displayTime: 4000,
//           title: 'Welcome!',  //set title:'header' below if you use 'ui message' instead of 'ui toast'
            message: 'I am a toast, nice to meet you! this is a lot of text and should be displayed properly',
            class: 'brown',
            className: {
                toast: 'ui compact message',
//                title: 'header'   //remember to set this aswell when using a title in a message
            },
            showProgress: 'bottom',
            progressUp: false
        });

Animated stacking on close transition: closeEasing = 'easeOutBounce'

toast_closeeasing

$('body')
          .toast({
            displayTime: 2000,
            message: 'I am a toast, nice to meet you! this is a lot of text and should be displayed properly',
            class: 'purple',
            compact: false, //do not restrict to a fixed width
            className: {
                toast: 'ui message',
            },
            transition: {
                  closeEasing:'easeOutBounce', //this is default, you can omit it
//               closeEasing:'', //set to empty string to gain the previous behaviour without any animation
            },
            showProgress: 'bottom'
        });

Top Progressbar showProgress: 'top'

toast_top_progress

$('body')
          .toast({
            title: 'Welcome to a fresh Toast',
            displayTime: 4000,
            message: 'I am a toast, nice to meet you! this is a lot of text and should be displayed properly',
            class: 'error',
            className: {
                toast: 'ui floating toast',
            },
            showProgress: 'top'
        });

Close Icon closeIcon: true

Difference to usual toast: It's only closable by close-icon instead of clicking anywhere in the toast. cursor changed accordingly
toast_close_pointer

Individual Icon showIcon: 'your icon name'

Using a string instead of true for showIcon will use that string for the icon display. Using true remains the old behaviour (displaying the appropriate icon which matches the class)

showIcon: 'accessible'
showIcon: 'microphone'

individual_icon

@ColinFrick ColinFrick added type/feat Any feature requests or improvements lang/css Anything involving CSS lang/javascript Anything involving JavaScript state/awaiting-docs Pull requests which need doc changes/additions state/awaiting-reviews Pull requests which are waiting for reviews labels Sep 28, 2018
@ColinFrick
Copy link
Member

Thanks for the pull request. Could you change the base branch to beta?

@lubber-de lubber-de changed the base branch from master to beta September 28, 2018 13:36
@lubber-de
Copy link
Member Author

Thanks for the pull request. Could you change the base branch to beta?

Done ;)

@y0hami
Copy link
Member

y0hami commented Sep 28, 2018

@lubber-de Great PR 👍, I will take a look and do some testing when I get some time.

@prudho
Copy link
Contributor

prudho commented Sep 28, 2018

Wow, this is a great PR ! Actually you did a lot of enhancements and fixes I was willing to do the next week 😄 As there's some big changes, I'll take the time to heavily test it, and I will take some time to do so. But from what I can see now, you seem to have done a great job !

}

.floating.toast {
box-shadow: 0 0 0 1px #a3c293 inset, 0 2px 4px 0 rgba(34,36,38,.12), 0 2px 10px 0 rgba(34,36,38,.15);
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this ! Since we got rid of opacity, what do you think about display it (or a variant) on mouse over ?

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 suggest implementing this as an additional class, so the user can decide how it should behave

Will only show the box-shadow on hover

className: {
	toast: 'ui hoverfloating toast'
}

Will still show the box shadow any time

className: {
	toast: 'ui floating toast'
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed / added accordingly by commit f49a63c.

prudho
prudho previously approved these changes Oct 1, 2018
Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

Alright, this looks good to me now !

Thank you for beeing comprehensive and patient. The toast is my first FUI module and I want it to be evolutive and perfect 😄

Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

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

I can't get the css to build after applying the patch:
'variable @floatingBoxShadow is undefined in file toast.less
after fixing that:
'variable @activePulseDuration is undefined in file toast.less

Sadly you can't use variables from other modules/elements that easily.

Rendering different sized toasts results in a progress bar outside of the the smaller toasts:
grafik

I just executed all three examples (from the description) at the same time.

…attached, made compact an option, fixed alignments, fixed responsiveness margins, removed unused variables
@lubber-de
Copy link
Member Author

Meanwhile, while you are testing my latest commit 😉 :
What do you think about having the option "showProgress" support either false or 'top' or 'bottom' (instead of only true/false) as an addition to the current only possibility to use a top progressbar by defining className: {progress:'ui top attached active progress'} ?
I think showProgress: 'top' is easier to use (and even less needed code if we maybe set showProgress:'bottom' to default.) If needed one can still use an own className definition.

@y0hami
Copy link
Member

y0hami commented Oct 4, 2018

@lubber-de I totally agree with this. I think it should be showProgress but default to false but allow false, true, 'top' and 'bottom'

@ColinFrick
Copy link
Member

@lubber-de would make sense. Definitely one thing I didn't really like of the original commit.

@y0hami
Copy link
Member

y0hami commented Oct 4, 2018

@etshy Good point. I said to add the boolean values so it would break any changes but we haven't released this yet so what is there to break 🤔 so we could just have 'top' and 'bottom'

@lubber-de
Copy link
Member Author

@etshy
Yes, it's actually not a difference right now. What i could add is to explicitely set bottom and remove top when showProgress: 'bottom' has been defined. Then the difference to true would be that true simply uses the given settings.className.progress value without any change. Of course, by default this is also 'ui bottom attached active progress', but could be changed to something different by the user at runtime and toast will leave that untouched then. 🤔

@etshy
Copy link

etshy commented Oct 4, 2018

Options should false (to not show the progress, ofc), 'top' and 'bottom' I guess.

@lubber-de I didn't take a look to the code or event the toast features yet so I don't really know what is settings.className.progress. A string to set classes to the progress bar ?
If yes, in the case you use showProgress: true, and don't specify a top or bottom class in settings.className.progress what happen ?

IMO, top or bottom should always be here so the 'top' or 'bottom' value is mandatory, either in settings.className.progress or in showProgress.

Let me know if I misunderstood something here.

@y0hami
Copy link
Member

y0hami commented Oct 4, 2018

@lubber-de @etshy I can see where you both are coming from but I think we should only have false, 'top' and 'bottom' it may mean a little more code but at the end of the day, it should be straightforward and easy to understand as a user.

default:
showProgress: false

available:
showProgress: false|'top'|'bottom'

@lubber-de
Copy link
Member Author

Alright, but if somebody sets this to true we should raise an exception on the console then?

@lubber-de
Copy link
Member Author

I think, I simply add the string value of showProgress to the div and if it's any boolean value, the progress won't be rendered completely. This way the user would also be able to add more classes to the progress if needed

@etshy
Copy link

etshy commented Oct 4, 2018

@hammy2899 that's how I thought it would be better, just false, 'top' and 'bottom'.

I was just asking, IF there is a true value, how it will be handled.

I guees you could raise an exception if true or any others value is set, and not show anything ?
I don't know if it's possible to do that with the current code though.

Thanks a lot to work so much on this 👍

@ColinFrick ColinFrick added this to the 2.6.x milestone Oct 5, 2018
@ColinFrick
Copy link
Member

@lubber-de is this ready for review now, or do you have more changes planned?

@lubber-de
Copy link
Member Author

@ColinFrick It's ready now. I have more additions in progress, but this can be a separate PR then. I won't touch this branch anymore unless you'll need more fixes/changes

@lubber-de lubber-de mentioned this pull request Oct 8, 2018
Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label Oct 10, 2018
@y0hami y0hami merged commit 409e533 into fomantic:beta Oct 10, 2018
@y0hami
Copy link
Member

y0hami commented Oct 10, 2018

If someone has some time and could do the docs this could be in our next release.

@lubber-de lubber-de deleted the toast_enhancements branch October 10, 2018 22:10
@lubber-de lubber-de added state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo and removed state/awaiting-docs Pull requests which need doc changes/additions labels Oct 15, 2018
@y0hami y0hami mentioned this pull request Oct 17, 2018
@lubber-de lubber-de modified the milestones: 2.6.x, 2.6.3 Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS lang/javascript Anything involving JavaScript state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants