Customizer: Alert on successful save to Gist #10632

stuartpb · 2013-09-14T08:12:45Z

Part of my suggested solution for #9951

cvrebert · 2013-10-11T06:23:19Z

assets/js/customizer.js

@@ -12,6 +12,12 @@ window.onload = function () { // wait for load in a dumb way because B-0
    throw err
  }

+  function showSuccess(msg {


You seem to be missing a close-paren...Has this code been tested?

Whoops! The built-in tests for Bootstrap don't lint the docs-assets. I've opened a PR for that at #11033.

cvrebert · 2013-10-11T06:27:17Z

IMHO, it'd be better to include the Gist and/or Customizer URLs in a README file and/or in the generated CSS+JS files.

stuartpb · 2013-10-11T06:35:20Z

From the discussion in the original issue:

My thinking is that waiting for the potential server response to concatenate the customizer / Gist URLs to the relevant files would unnecessarily slow down and complicate the file output flow:

If a developer just wants a snapshot of their selected options, the included config.json gives that to them.

If a developer wants to keep a note of where the configuration is stored online, they can copy the URLs from the callout to the output's comment block(s), or wherever they're keeping their development notes.

If a developer's Bootstrap development flow is complex enough to merit repeated output of the Gist URL to their output comments every time, they should probably be doing their Bootstrap customization / compilation locally instead of using the online customizer anyway.

Anyway, incorporating this change wouldn't stop anybody from implementing that - it would just give more visual feedback as to the existence of these links.

cvrebert · 2013-10-11T06:39:44Z

I guess I don't agree with your assessment of the potential complexity+performance impact.

stuartpb · 2013-10-11T06:53:30Z

I'll make a pull request some time with the performance reasons why I'd argue against adding it. Anyway, I'd still implement it on top of this change.

stuartpb · 2013-10-11T07:14:22Z

Pushed a rebased and revised commit.

ssorallen · 2013-11-04T19:37:07Z

docs-assets/js/customizer.js

+      var hereNow = window.location.origin + window.location.pathname + '?id=' + result.id
+      showSuccess('<strong>Success!</strong> Your configuration has been saved to <a href="' + gist +'">' + gist + '</a>\
+        and can be revisited here at <a href="' + hereNow +'">' + hereNow + '</a> for further customization.')
+      history.replaceState(false, document.title, hereNow)


Should this check for the existence of window.history before calling it? IE9 does not implement the history API: http://caniuse.com/#search=history

I think it's being polyfilled? If not, that's an issue that exists upstream.

IE9 does not work anyway due to jszip's limitations.

cvrebert · 2014-04-04T00:08:07Z

@stuartpb I believe we'd be happy to accept this if it was rebased/redone so that it merges cleanly against current master.

hnrch02 · 2014-05-05T22:14:14Z

@stuartpb Ping.

stuartpb · 2014-05-12T15:51:12Z

Rebased, updated and pushed.

ftbastler · 2014-05-12T15:57:35Z

+1

XhmikosR · 2014-05-12T16:01:33Z

@stuartpb: Tests fail.

stuartpb · 2014-05-12T16:10:11Z

Apparently Bootstrap implemented linting of docs-assets in testing, with concatenation over newline escapement for multi-line strings, since I originally submitted this PR. Fixed.

XhmikosR · 2014-05-12T16:13:52Z

Thanks. Can you squash those 2 commits into one?

stuartpb · 2014-05-12T16:23:06Z

Squashed and pushed.

stuartpb · 2014-05-12T16:23:49Z

BTW, when did doc asset linting get added to the test suite? I proposed doing so in #11033 and got shot down.

XhmikosR · 2014-05-12T16:26:43Z

I added it some time ago.

cvrebert · 2014-05-12T16:31:20Z

Lint still failing. This time due to lack of a space between the + operator and '">'.

stuartpb · 2014-05-12T17:57:33Z

Fixed, tests passing.

fat · 2014-05-13T04:23:35Z

docs/assets/js/_src/customizer.js

@@ -60,10 +66,13 @@ window.onload = function () { // wait for load in a dumb way because B-0
      data: JSON.stringify(data)
    })
    .success(function (result) {
+      var gistUrl = result.html_url;


stuartpb · 2014-05-13T04:53:11Z

Removed all semicolons per @fat's complaint

cvrebert · 2014-05-13T10:32:46Z

Will merge as soon as we get a chance to test it out.

XhmikosR · 2014-05-15T16:07:48Z

I had a look at this. After doing grunt dist and trying the patch locally I get:

XhmikosR · 2014-05-15T16:14:47Z

OK, this patch is pretty old... the classes no longer exist.

@stuartpb: can you update the patch to use bs-callout bs-callout-info and use a similar markup as showCallout?

stuartpb · 2014-05-17T17:23:35Z

How's this change?

XhmikosR · 2014-05-19T06:27:10Z

It's better for sure.

What I'm not sure about is the duplication. I mean, we have showError, showCallout and now showSuccess. I think we should at least follow the same markup as the other two functions meaning using h4 and p.

After this is in, we can extend an existent function instead of duplicating stuff.

stuartpb · 2014-05-19T06:34:02Z

No close button?

XhmikosR · 2014-05-19T06:44:48Z

The close button should be there I guess (see also https://github.com/twbs/bootstrap/pull/10632/files#diff-d16c8aeafb7656f16175f3eebc03c831R19).

cvrebert · 2014-05-19T07:04:20Z

Given #12955, making the alerts always dismissable could be misleading.

XhmikosR · 2014-05-19T07:20:35Z

Yeah but that is a general issue and not directly related to this. I simply suggested we are a little more consistent with the alerts so far.

cvrebert · 2014-06-05T20:57:53Z

Looks good enough, IMO. We can always tweak it more later.

Customizer: Alert on successful save to Gist

cvrebert · 2014-06-05T20:58:38Z

Thanks @stuartpb!

mdo · 2014-06-05T21:06:45Z

✨

stuartpb mentioned this pull request Oct 10, 2013

Put the URL with the Custom Build ID in the generated source files #9951

Closed

cvrebert reviewed Oct 11, 2013
View reviewed changes

stuartpb mentioned this pull request Oct 11, 2013

Lint doc-assets like the customizer in testing #11033

Closed

ssorallen reviewed Nov 4, 2013
View reviewed changes

mdo modified the milestones: v3.2.0, v3.1.1 Feb 10, 2014

fat reviewed May 13, 2014
View reviewed changes

Customizer: Alert on successful save to Gist

b5f9e2e

Update showSuccess to use new classes

d5d64c2

cvrebert added a commit that referenced this pull request Jun 5, 2014

Merge pull request #10632 from stuartpb/patch-1

a8641b4

Customizer: Alert on successful save to Gist

cvrebert merged commit a8641b4 into twbs:master Jun 5, 2014

mdo mentioned this pull request Jun 5, 2014

v3.2.0 ship list #12736

Closed

Customizer: Alert on successful save to Gist #10632

Customizer: Alert on successful save to Gist #10632

Conversation

stuartpb commented Sep 14, 2013

cvrebert Oct 11, 2013

Choose a reason for hiding this comment

stuartpb Oct 11, 2013

Choose a reason for hiding this comment

cvrebert commented Oct 11, 2013

stuartpb commented Oct 11, 2013

cvrebert commented Oct 11, 2013

stuartpb commented Oct 11, 2013

stuartpb commented Oct 11, 2013

ssorallen Nov 4, 2013

Choose a reason for hiding this comment

stuartpb Nov 5, 2013

Choose a reason for hiding this comment

XhmikosR Feb 4, 2014

Choose a reason for hiding this comment

cvrebert commented Apr 4, 2014

hnrch02 commented May 5, 2014

stuartpb commented May 12, 2014

ftbastler commented May 12, 2014

XhmikosR commented May 12, 2014

stuartpb commented May 12, 2014

XhmikosR commented May 12, 2014

stuartpb commented May 12, 2014

stuartpb commented May 12, 2014

XhmikosR commented May 12, 2014

cvrebert commented May 12, 2014

stuartpb commented May 12, 2014

fat May 13, 2014

Choose a reason for hiding this comment

stuartpb commented May 13, 2014

cvrebert commented May 13, 2014

XhmikosR commented May 15, 2014

XhmikosR commented May 15, 2014

stuartpb commented May 17, 2014

XhmikosR commented May 19, 2014

stuartpb commented May 19, 2014

XhmikosR commented May 19, 2014

cvrebert commented May 19, 2014

XhmikosR commented May 19, 2014

cvrebert commented Jun 5, 2014

cvrebert commented Jun 5, 2014

mdo commented Jun 5, 2014