Tweak Quick Edit error strings & update docs#7349
Conversation
- Fix comments to reflect Quick Edit error related API changes - Fix outdated FileSyncManager docs
There was a problem hiding this comment.
Removed "provider" since it's an implementation term that probably won't mean much to end users
|
Thanks for reviewing these strings. The changes all look like improvements, except maybe the 1 case I noted. I won't be around to merge this change. |
There was a problem hiding this comment.
I don't mind comments like this in the source but, for documentation, I don't think the API should feel like it has a pulse. Comments like "We can't determine the context" are fine in logic branches but in JSDoc it feels a little too folksy...
There was a problem hiding this comment.
There are already well over 1000 occurrences of "we" in our code today, so I think the horse maybe has already left the barn on that one :-) And this sentence was already structured that way...
There was a problem hiding this comment.
Yes, that was my veiled attempt to get you to re-word since you added a second "we" to the sentence making it feel a little sarcastic and folksy but it's also somewhat ambiguous with the 2 references to "we" : "we're not even close to to a context where we could..."
Maybe "The insertion point is not close enough to a context where we could provide..." is a little less ambiguous. If you wanted to keep it folksy you could say "You're not even close enough to a context where we could..." but that might be too folksy :)
There was a problem hiding this comment.
@JeffryBooher Change pushed that rewords this a little bit. Lmk if you need anything more to merge.
…e for cursor pos being ambiguous between two class names. Fix some more JSDoc typos found in code review.
|
@njx / @TomMalbran changes pushed. Since @redmunds isn't around for re-review, can one of you vouch for this & merge? (btw, the diff in the last commit will look cleaner with |
|
Actually @JeffryBooher is the assigned reviewer - since he looked at it before, seems like he should finalize it. |
|
Feels like https://trello.com/c/YEd0m9Il should be higher on the backlog now. I really want to invoke the color picker before typing anything now... |
There was a problem hiding this comment.
This is elegant code but wouldn't be easier and more performant to just check to see if the characters before & after tagInfo.position.offset are spaces and return between class names?
There was a problem hiding this comment.
actually disregard that. I guess this is fine. the fact that you added a call to .trim() and checked the result to make sure it wasn't an empty attribute made me think that this was all new code. Was there a reason to add the call to trim()? Was it just to avoid the logic in the event that it was class=" " which would probably need its error string that we should add...
There was a problem hiding this comment.
n/m that case is covered. what's confusing here is that the name is ERROR_CSSQUICKEDIT_CLASSNOTFOUND but the error displayed is "incomplete class attribute" which I guess is consistent with ID check below.
There was a problem hiding this comment.
Stealth comment: there may be whitespace other than the space char, so instead of using lastIndexOf(" ") I think a regex with \s should be used.
There was a problem hiding this comment.
Also, using trim() here is inefficient because it creates a new string and then manipulates it to remove the whitespace at start/end, where all you really want to do is to detect if string contains any non-whitespace characters. It's faster to use regex to check for existence of any \S.
|
done with final review. just the 2 things to consider and this should be good to go. correction: just the 1 thing I guess. It's not a big deal. LMK if you think it's too much. In general some of the constants are a little misleading such as "CLASS_NOT_FOUND" when we display "incomplete class attribute". Seems that the 2 should agree. |
|
I tested this and compared it to sprint 27 and it's much more friendly. I tested a few other scenarios as well and it feels pretty solid and it's a low-impact change. I'm ready to merge this if you don't want to fix that one comment I mentioned let me know and I will merge it. |
|
@JeffryBooher I think it's probably ok as is -- it's the same constant name used in the original PR that was already merged, and it didn't closely match what the user-visible string said back then either. Re the other comments above, just to note: most of that code hasn't changed in this diff, it's just been indented. If you add |
|
wow @peterflynn where do you find this stuff :) that works wonderfully! Thanks for the tips on github. |
|
@peterflynn do you want to change the comment mentioned above? |
|
looks good. Merging. |
Tweak Quick Edit error strings & update docs
5 participants
@redmunds, would you be able to review?
CC @njx in case of feedback about the string changes