Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Display Inline Editor Errors#6916

Merged
JeffryBooher merged 24 commits intomasterfrom
randy/display-inline-editor-errors
Mar 20, 2014
Merged

Display Inline Editor Errors#6916
JeffryBooher merged 24 commits intomasterfrom
randy/display-inline-editor-errors

Conversation

@redmunds
Copy link
Copy Markdown
Contributor

This has been implemented using proposal outlined here.

The Quick Docs Feature uses the same mechanism, so it is also supported.

Code handles errors for the CSS, JS, and InlineTimingFunction Inline Editors. Are there any cases that could be handled for the InlineColorEditor? Any other Inline Editors?

TODO:

  • immediately dismiss message on:
    • when displaying another inline editor error
    • on scroll
    • change editors
    • selection change
    • edit page
  • scroll cursor into view
  • unit tests
  • determine conflicts with other floating thingies
    • Quick View
    • other Quick Edit or Quick Docs widget
    • main menus (HTML & native)
    • context menu
    • dropdowns (New Rule?)
  • UX review
  • string review
  • any InlineColorEditor error cases that can be handled?
  • code review
  • rebase
  • update quick edit API documentation

@redmunds
Copy link
Copy Markdown
Contributor Author

@larz0 Can you do UX review? Let me know if you review the strings or if someone else should do that.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Feb 19, 2014

@redmunds reviewing now.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Feb 19, 2014

We need to use the same initial transition as the quick views, the end transition is great. Could we try 2secs instead of 3? Let's make strings shorter as well; could we try:

"Oops, the value is invalid."
"Oops, Quick Edit can't be opened here."
"Oops, Quick Doc can't be opened here."

Maybe @njx has better idea for the strings.

@redmunds
Copy link
Copy Markdown
Contributor Author

@larz0

Could we try 2secs instead of 3?

I started with a 3s duration for displaying message, but someone requested 6s so I bumped it up to the current value of 5s. Should I go back to 3s to see how that feels?

@larz0
Copy link
Copy Markdown
Member

larz0 commented Feb 19, 2014

Okay cool let's try that.

@redmunds
Copy link
Copy Markdown
Contributor Author

@larz0 I implemented the transition on open.

But, I did not change the duration message is shown (it's still 5 seconds) because the first time someone sees it they may need more than 3 seconds (that we discussed) between the time it catches their attention and they read it. Instead I implemented the following ways to dismiss message box immediately so people who have seen don't have to wait for it:

  • when another message box is being displayed
  • click somewhere in page
  • edit page
  • scroll
  • change editors
  • Let me know if you think of any other cases

Let me know what you think.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Feb 27, 2014

@redmunds that's awesome. I found an edge case where if you cmd+e on the same spot again you won't be able to dismiss it via clicking if you've done that the first time.

@redmunds
Copy link
Copy Markdown
Contributor Author

@larz0 that edge case should be fixed.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Feb 27, 2014

It's still the same here after I pulled that in. When I click on the tooltip to dismiss it on the same spot for the second time it won't go away. Not sure if it's just me.

@redmunds
Copy link
Copy Markdown
Contributor Author

It doesn't actually listen for "click in page" it listens for "selection change" -- I updated description above. So it doesn't dismiss if you click in same exact position -- is that OK?

@larz0
Copy link
Copy Markdown
Member

larz0 commented Feb 27, 2014

Yeah I think that's fine.

@redmunds
Copy link
Copy Markdown
Contributor Author

@larz0 Are you done with the UX review? If not, let me know what issues remain.

@redmunds
Copy link
Copy Markdown
Contributor Author

@njx Can you do the string review?

@larz0
Copy link
Copy Markdown
Member

larz0 commented Mar 11, 2014

Yep I'm done with the UX review. 👍

@redmunds
Copy link
Copy Markdown
Contributor Author

@JeffryBooher I merged in the latest master. If you think there's enough work here, then go ahead and create a Kanban card for this.

@JeffryBooher
Copy link
Copy Markdown
Contributor

Ran unit tests and a few scenarios. code looks good. Merging

JeffryBooher added a commit that referenced this pull request Mar 20, 2014
@JeffryBooher JeffryBooher merged commit 994daeb into master Mar 20, 2014
@JeffryBooher JeffryBooher deleted the randy/display-inline-editor-errors branch March 20, 2014 20:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants