Improved deep/full copying logic to address errors and provide user feedback #225
Conversation
…FeatureService` use. Fixed typo in `addItem.js`. Updated default `request` timeout to 5 minutes instead of 2. Added `update('allow_dismiss,...)` to bootstrap-notify.min.js.
| @@ -20,6 +21,28 @@ require([ | |||
| Clipboard | |||
| ) { | |||
|
|
|||
| var unsaved = {}; | |||
| var isDirty = function() { | |||
There was a problem hiding this comment.
Comments on new functions would be helpful in explaining their role.
| display: flex; | ||
| } | ||
|
|
||
| #itemsArea { |
There was a problem hiding this comment.
Is there a compelling reason for hard-coding layout sizes instead of using the responsive elements in Bootstrap?
There was a problem hiding this comment.
Yes, the primary reason is because Bootstrap's grid system presents 2 challenges for our UI:
- At certain page widths (the most common from what I've seen), Bootstrap's grid system places a margin on the left/right side of the main content area (centered) and leaves a fair amount of space left unavailable. This is particularly relevant with regards to JSON viewing/editing. The new code removes this unused space.
- The Bootstrap grid framework defaults to a float implementation when screen size is reduced (i.e. to a typical mobile size) which either causes the right
#dropAreato float down (below the#itemArea, and thus become unreachable) or makes either column unusable because the column elements are too small. It also causes thenavbarto grow in height and thus cover up the top portion of either content columns.
Basically, there are lots of issues with our UI/UX css currently, and these changes are meant to address some of them before we get to the point where we can look at a full UI refresh down the road.
There was a problem hiding this comment.
The layout changes you're proposing will have implications across the entire app. They should be broken out to a separate branch and PR so we can have a more focused discussion on those specific changes.
Regarding your comments, screenshots would really be helpful. Both 1 and 2 should be addressable by making better use of Bootstrap's media breakpoints. I'm not opposed to migrating away from those in favor of something else as long as it provides flexibility and responsiveness. Hardcoded css rules make both of those harder.
I went through this one with @slibby trying to trace the origin. The same property shows up on this method in several other apps and scripts. It's certainly possible that I inadvertently copied the code between them all, but I have a different theory. I believe it is not a typo, but rather that the API spec changed at some point. This would explain why the method works fine as is (just with some unintended side-effects on the back end). This is a good example of why detailed (and versioned) API documentation is needed. Ideally, Esri can start detailing API changes with each release. Github's developer blog is a particularly good example of how to do this well. |
…eld names must be all lower case 2) Layer ID of destination doesn't necessarily match the ID of the source. This commit incorporates fixes for both scenarios.
…it had been created). Addding missing images to fix 404s
|
@ecaldwell are you ok with a 'don't let the perfect be the enemy of good' if we can address the CSS issues? |
|
@slibby Yep...the hardcoded CSS rules in this PR are my primary concern. The other functionality looked good the last time I put eyes on this. |
|
@hogpilot do you think this is still good to merge? A lot of time has passed but as I recall at the time it was working quite well. |
Lots of new code here which will require thorough review and testing. It should throttle deep copying to limit asynchronous requests to 5 simultaneous (a sweet spot found during my testing). There are a few other improvements in here for CSS,
addItemtypo (from way back), a notification window with a copy progress bar, and a warning if the user tries to close/leave the page while copying is in progress.I think this may fix #224 #215 #210 #172