Replace JavaScript alert/confirmations with sweetalert2 #1035

sacr3dc0w · 2017-07-02T06:02:15Z

What?

I’ve used this for a few projects and wanted to make it accessible to the community. I’ve cleaned it up and done my best to make it conform to Cornerstone stylings but maybe some additional eyes can fill in any missing blanks. SweetAlert2 is described by their webpage as:

A beautiful, responsive, customizable, accessible (WAI-ARIA) replacement for JavaScript's popup boxes. Zero dependencies.

Documentation

The project’s webpage goes into deeper detail on it’s usage.

https://sweetalert2.github.io/

Screen Recordings

Example

Remove item from cart, desktop (all theme variations)

Remove item from cart, tablet (all theme variations)

Remove item from cart, mobile (all theme variations)

Compare page error

Compare product error on category page

lord2800

Seems good to me.

mjschock · 2017-07-05T22:59:15Z

@sacr3dc0w overall it looks good to me. my concern is the file size:

i see nearly 50k for this. however, it turns out sweetalert2's package main is pointing to dist/sweetalert2.js. modifying that to point to dist/sweetalert2.min.js gets a more reasonable profile:

can you please add another alias in webpack.conf.js to map to the min file, like so:

sweetalert2:  path.resolve(__dirname, 'node_modules/sweetalert2/dist/sweetalert2.min.js'),

mjschock

overall LGTM - just need to update the webpack resolve to use the minified version

davidchin

Hey @sacr3dc0w, nice work. Thank you for the screenshots. The alert does look better! I just have few minor comments I want you to look at 😄

davidchin · 2017-07-06T00:01:16Z

assets/js/theme/global/compare-products.js

@@ -57,7 +58,10 @@ export default function (urlContext) {
        const productsToCompare = $this.find('input[name="products\[\]"]:checked');

        if (productsToCompare.length <= 1) {
-            alert('You must select at least two products to compare');
+            swal({
+                text: 'You must select at least two products to compare',


This is an existing problem. So you probably don't need to fix it in this PR. Just pointing it out that the warning message needs to come from the language file, not hard-coded in here.

davidchin · 2017-07-06T00:15:03Z

assets/js/theme/cart.js

+                const delta = new Date() - openTime;
+
+                // Delta workaround for Chrome's "prevent popup"
+                if (!result && delta > 10) {


A question. Do we need this hack anymore? It looks like sweetalert2 is just a regular modal. Unlike window.confirm, the browser might not interfer.

davidchin · 2017-07-06T00:20:53Z

assets/scss/settings/vendor/sweetalert2/_settings.scss

+$swal-font-headingsColor:                             stencilColor("color-textHeading");
+
+// Modal stylings
+$swal-modal-bgColor:                                  stencilColor("navUser-dropdown-backgroundColor") !important;


You should probably introduce a new variable instead of using navUser-dropdown-backgroundColor? It looks like navUser-dropdown-backgroundColor is intended to used for navigation dropdowns only.

davidchin · 2017-07-06T00:24:18Z

assets/scss/settings/vendor/sweetalert2/_settings.scss

+$swal-button-wrapperMargin:                           25px 0 0;
+
+// Confirm button
+$swal-button-bgColor:                                 stencilColor("button--primary-backgroundColor") !important;


Do you really need to use !important here? If the styles are cascaded properly, you shouldn't need to increase their specificity.

davidchin · 2017-07-06T00:31:21Z

assets/js/theme/common/state-country.js

@@ -129,7 +130,10 @@ export default function (stateElement, context = {}, options, callback) {

        utils.api.country.getByName(countryName, (err, response) => {
            if (err) {
-                alert(context.state_error);
+                swal({


Do you want to only execute the callback after dismissing the alert? I think sweetalert2 is non-blocking, unlike alert, which stops the execution the script until it is dismissed.

junedkazi · 2017-07-06T16:57:28Z

This is nice but I have been thinking if it makes sense to move this functionality to https://github.com/bigcommerce/stencil-utils so we can build a wrapper around it. It will make it much more easier to change the underlying library in the future & also to upgrade the lib in the future to propagate the changes across the board by just updating the package version.

mjschock · 2017-07-06T17:20:12Z

i like @junedkazi 's idea of abstracting it as utils.alert from stencil.utils. i'm not sure where exactly it would fit in there though - perhaps in a components folder? it would be nice down the line to have the specifics of the theme 'swappable' so to speak. maybe we could even ultimately swap in different frameworks.

davidchin

@junedkazi good idea! I'm not against moving the alert function in stencil-utils which internally invokes sweetalert2, if you think it will be useful for all theme developers. I'm also fine leaving it here for now and moving it out later when there's a need.

Anyway, when you create the facade, make sure it is well encapsulated so that it doesn't leak implementation details (i.e.: any references to sweetalert2 cannot be exposed, including CSS classes).

PS: A question. It seems stencil-utils doesn't really have a clear responsibility. Is it just a general purpose package?

davidchin · 2017-07-06T21:16:16Z

config.json

@@ -177,6 +177,7 @@
    "checkRadio-color": "#4f4f4f",
    "checkRadio-backgroundColor": "#ffffff",
    "checkRadio-borderColor": "#dfdfdf",
+    "sweetAlert-backgroundColor": "#ffffff",


Can we use a more generic config name? This name ties to a specific vendor library. Maybe just alert-backgroundColor.

davidchin · 2017-07-06T21:20:24Z

assets/scss/settings/vendor/sweetalert2/_settings.scss

+// -----------------------------------------------------------------------------
+
+// Font color, size, family
+$swal-font-body:                                      stencilFontFamily("body-font"), Arial, Helvetica, sans-serif;


All these variables can probably be generalised as well. i.e.: $alert-font-body

davidchin · 2017-07-06T21:23:01Z

assets/js/theme/account.js

@@ -105,7 +107,10 @@ export default class Account extends PageManager {

            if (!submitForm) {
                event.preventDefault();
-                alert('Please select one or more items to reorder.');
+                swal({


Can we have a intention-revealing function name? swal doesn't communicate anything.

davidchin · 2017-07-06T21:24:58Z

assets/js/theme/account.js

@@ -5,6 +5,7 @@ import Wishlist from './wishlist';
 import validation from './common/form-validation';
 import stateCountry from './common/state-country';
 import { classifyForm, Validators, insertStateHiddenField } from './common/form-utils';
+import swal from 'sweetalert2';


You may choose to not import the vendor library directly. Instead, create a facade so that it's easier to change its implementation later.

davidchin

Thanks for doing the changes 👍. I'm happy with the current state. And do refactoring later.

mjschock · 2017-07-20T23:42:13Z

@sacr3dc0w - could you please update the changelog to include an entry for these changes in the drafts section - i'll merge it in after that

limonte · 2018-01-06T09:44:49Z

https://limonte.github.io/sweetalert2/

this link is outdated, here's the correct one https://sweetalert2.github.io/

PreciselyAlyss added the community pull request label Jul 5, 2017

PreciselyAlyss requested a review from davidchin July 5, 2017 22:14

jasonherbst8 requested review from mjschock, junedkazi and lord2800 July 5, 2017 22:16

lord2800 approved these changes Jul 5, 2017

View reviewed changes

mjschock suggested changes Jul 5, 2017

View reviewed changes

davidchin reviewed Jul 6, 2017

View reviewed changes

davidchin approved these changes Jul 12, 2017

View reviewed changes

mjschock approved these changes Jul 20, 2017

View reviewed changes

sweetalert2 to replace JavaScript alert/confirmations

f35e6ee

mjschock merged commit 4e79c07 into bigcommerce:master Jul 21, 2017

sacr3dc0w deleted the sweetalert2 branch July 21, 2017 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace JavaScript alert/confirmations with sweetalert2 #1035

Replace JavaScript alert/confirmations with sweetalert2 #1035

sacr3dc0w commented Jul 2, 2017 •

edited

Loading

lord2800 left a comment

mjschock commented Jul 5, 2017

mjschock left a comment

davidchin left a comment

davidchin Jul 6, 2017

davidchin Jul 6, 2017

davidchin Jul 6, 2017

davidchin Jul 6, 2017

davidchin Jul 6, 2017

junedkazi commented Jul 6, 2017 •

edited

Loading

mjschock commented Jul 6, 2017

davidchin left a comment

davidchin Jul 6, 2017

davidchin Jul 6, 2017

davidchin Jul 6, 2017

davidchin Jul 6, 2017

davidchin left a comment

mjschock commented Jul 20, 2017

limonte commented Jan 6, 2018

Replace JavaScript alert/confirmations with sweetalert2 #1035

Replace JavaScript alert/confirmations with sweetalert2 #1035

Conversation

sacr3dc0w commented Jul 2, 2017 • edited Loading

What?

Documentation

Screen Recordings

Example

lord2800 left a comment

Choose a reason for hiding this comment

mjschock commented Jul 5, 2017

mjschock left a comment

Choose a reason for hiding this comment

davidchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junedkazi commented Jul 6, 2017 • edited Loading

mjschock commented Jul 6, 2017

davidchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchin left a comment

Choose a reason for hiding this comment

mjschock commented Jul 20, 2017

limonte commented Jan 6, 2018

sacr3dc0w commented Jul 2, 2017 •

edited

Loading

junedkazi commented Jul 6, 2017 •

edited

Loading