search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] Add a new flag "isNewItem" to css info object used in CSS Utils API. #2254

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by RaymondLim Friday Dec 14, 2012 at 23:48 GMT Originally opened as https://github.com/adobe/brackets/pull/2364

If the new flag is true, then it indicates a new insertion at the location of "index". Otherwise, the value of "index" points to the existing item in the "values" array.

RaymondLim included the following code: https://github.com/adobe/brackets/pull/2364/commits

core-ai-bot commented 3 years ago

Comment by njx Saturday Dec 15, 2012 at 00:44 GMT

There are some core cases that seem to be broken in this pull request:

    width:|
    width: |
    width:|;
    width: |;
    font-family: Arial,|
    font-family: Arial,|;
    font-family: Arial, |;

(where | is the cursor position). In all these cases, isNewItem is false, but I would expect it to be true. (The case font-family: Arial, | (with no semicolon) returns true for isNewItem).

core-ai-bot commented 3 years ago

Comment by peterflynn Saturday Dec 15, 2012 at 01:20 GMT

Would it be more correct to have isNewItem be consistently false in all the "Arial, " cases? The user story says comma-separated values are out of scope, so I assume we'd naively treat font-family: Arial, the same as color: red. Or do we need this for EWF to work correctly?

core-ai-bot commented 3 years ago

Comment by njx Saturday Dec 15, 2012 at 01:53 GMT

The current code is attempting to deal properly with the comma-separated cases, and it's working consistently in all the other cases besides this one. So I don't think we should rip that out at this point. (I think the fix in this case is going to be the same as the non-comma cases.)

core-ai-bot commented 3 years ago

Comment by RaymondLim Saturday Dec 15, 2012 at 04:04 GMT

@njx All the failing cases are fixed. I missed to convert one place that I added an empty string. Previous commit just removed the code that is adding the empty string. Now I'm setting the flag to add new item. So the logic is now exactly the same as the one before we add a new flag.

@peterflynn I also fixed the jsDoc for optional parameters.

core-ai-bot commented 3 years ago

Comment by njx Monday Dec 17, 2012 at 18:21 GMT

To@redmunds. The main change Raymond made here was to make it so we don't create an empty string in the values array for a multi-valued property. Instead, we just have the index point to the next item in the values array, and set the "isNewItem" flag so the caller knows they should be inserting a new item instead of replacing an existing one. I'll check later on today whether his most recent fixes fix all my unit tests.

core-ai-bot commented 3 years ago

Comment by redmunds Monday Dec 17, 2012 at 20:46 GMT

Made 1 small update. Looks good. Merging.

core-ai-bot commented 3 years ago

Comment by redmunds Monday Dec 17, 2012 at 20:47 GMT

@njx Raymond's changes merged.