seek-oss / html-sketchapp-cli

Quickly generate Sketch libraries from HTML documents and living style guides, powered by html-sketchapp
MIT License
633 stars 31 forks source link

Not able to perform consistent sync of Sketch Library File #18

Closed lassediercks closed 1 year ago

lassediercks commented 6 years ago

Hey there,

to be honest, I'm blown away how easily this turns my html into sketch symbols. Big props for that.

Let me explain what I'm struggling with:

  1. I turn a html file into the asketch.json file
  2. I trigger the sketch plugin and select both files
  3. I save that file
  4. I link the saved filed in the preferences as a library
  5. I create a new file and use the symbols from the library
  6. I update the html and perform step 1-3

I'd expect to have this image in my sketch but it does not appear. I'm able to use the updated symbols from the library but already used symbols are not updated.

Do you know this issue or how to fix it?

thanks for the awesome work <3

markdalgleish commented 6 years ago

Yeah, I was able to replicate this too. I'll have to find some time to dig into this a bit more and figure out what Sketch uses to decide whether a symbol is the same instance or not. In the meantime, if you have time to look into this too, that would be great. Once we've narrowed it down, it hopefully shouldn't be too hard to fix.

Also, thanks for the positive feedback! I really appreciate it 😄

markdalgleish commented 6 years ago

I had a look into this recently and realised it's quite tricky, and depends on whether or not html-sketchapp is able to generate consistent IDs. Any thoughts on this, @kdzwinel?

kdzwinel commented 6 years ago

Only IDs that come to my mind are objectIDs which ATM are randomly generated UUIDs. It should be possible to replace them with consistent ones. My guess is that Sketch only looks at the objectID of the symbol (and not its children), so that's the one I'd recommend playing with. I'd start by trying to set symbol._objectID (there is no public setter ATM) to something non-random. Not sure if it even has to be an UUID? Maybe any string will do? If so, symbol name should work.

I'd love to hear back if someone figures that one out!

markdalgleish commented 6 years ago

IIRC Sketch maintains a reference to both the symbol ID and the document ID.

kdzwinel commented 6 years ago

Ah, OK, but it looks like the document objectID is generated in the same way and can be replaced in the same way as for the symbol - document._objectID = '…'.

elliotdickison commented 6 years ago

I recently ran into this problem as well. I think fixing this is going to be pretty essential for maintaining a library over time. A couple thoughts:

markdalgleish commented 6 years ago

@kdzwinel not including a document ID in document.asketch.json makes sense to me. That threw me off for a while—I had to dig into the asketch2sketch source to realise that the document ID isn't used at all.

markdalgleish commented 6 years ago

@elliotdickison I'd say that hashing based on symbol name makes sense to me as a sensible default. In the long term, we could let you provide your own logic for generating IDs, which would allow you to provide backwards compatible hashes by aliasing symbol names, for example.

iamstarkov commented 6 years ago

we thought about it a bit and came up with this naming convention:

*.asketch.json proposal with relevant keys included:

{
  "do_objectID": "nordnet-ui-kit",
  "layers": [
    {
      "name": "Badge/default",
      "symbolID": "Badge/default",
      "do_objectID": "Badge/default",
      "layers": [
        {
          "do_objectID": "Badge/default-0",
          "layers": [
            { "do_objectID": "Badge/default-0-0" }
          ],
        },
        {
          "do_objectID": "Badge/default-1",
          "layers": [],
        }
      ],
    },
    {
      "name": "Badge/danger",
      "symbolID": "Badge/danger",
      "do_objectID": "Badge/danger",
      "layers": [
        {
          "do_objectID": "Badge/danger-0",
          "layers": [
            { "do_objectID": "Badge/danger-0-0" }
          ],
        },
        {
          "do_objectID": "Badge/danger-1",
          "layers": [],
        }
      ],
    },
    {
      "name": "Badge/warning",
      "symbolID": "Badge/warning",
      "do_objectID": "Badge/warning",
      "layers": [
        {
          "do_objectID": "Badge/warning-0",
          "layers": [
            {
              "do_objectID": "Badge/warning-0-0",
              "layers": [],
            }
          ],
        },
        {
          "do_objectID": "Badge/warning-1",
          "layers": [],
        }
      ],
    },
    {
      "name": "Badge/success",
      "symbolID": "Badge/success",
      "do_objectID": "Badge/success",
      "layers": [
        {
          "do_objectID": "Badge/success-0",
          "layers": [
            {
              "do_objectID": "Badge/success-0-0",
              "layers": [],
            }
          ],
        },
        {
          "do_objectID": "Badge/success-1",
          "layers": [],
        }
      ],
    }
  ],
}
iamstarkov commented 6 years ago

@markdalgleish, @brainly, @lassediercks, @mlenser, @HerrBertling, @ronalson what do you think?

lassediercks commented 6 years ago

I'd say if it works let's do it but from a technical point of view I cant judge the quality

iamstarkov commented 6 years ago

we will try it

iamstarkov commented 6 years ago

here we go https://github.com/brainly/html-sketchapp/pull/65

iamstarkov commented 6 years ago

and after that cli needs to be adjusted as well

kdzwinel commented 6 years ago

FYI html-sketchapp 3.2.0 has a proper ID setter on SymbolMaster now:

https://github.com/brainly/html-sketchapp/blob/cddaddadce4dcaeea02ec20755f0c2a3ff16eb0c/html2asketch/model/symbolMaster.js#L16-L18

@burakukula was able to successfully use it in Brainly style-guide import to have consistent symbols.

https://github.com/brainly/html-sketchapp-style-guide/blob/e4221b0e93582a4380f38f17c38045d0c7e5e5dd/src/styleguide2asketch.js#L49

markdalgleish commented 6 years ago

This is not a pressing issue for us internally. If anyone can offer some help implementing this, that would be much appreciated.

mnikkane commented 6 years ago

I think #52 will fix at least some related parts of this. We've had problems where symbols imported to sketch library from asketch file cannot really be updated correctly as they lose for example text set for buttons.

52 will allow to set symbol ID, which seems to make Sketch to handle component updates better and edited/added texts are not replaced with default texts (default = text in html).

We do still have some problems to have "Library update" available not appearing, but it is not critical to us as moving components for example a bit to the left and back to the right and then saving the file seems to trigger Sketch to notice update available.

kdzwinel commented 6 years ago

@mnikkane we haven't used library feature yet (although now that Sketch 51 supports text styles in libraries we might), so I haven't heard experienced the "Library update" issue you are describing yet. If there is something we can do in html-sketchapp to fix it - please open an issue for it.

We've had problems where symbols imported to sketch library from asketch file cannot really be updated correctly as they lose for example text set for buttons.

Instances of symbols loose overwritten text when master symbols are updated? We haven't experienced that 🤔Again, if there is antyhing we can do in html-sketchapp to fix it - let us know!

/cc @burakukula

mnikkane commented 6 years ago

I think the problem in our case was that we have a Sketch library (generated completely with html-sketchapp-cli) and then design files, which use components (like buttons) from the library. And in design files button texts etc. are of course edited (texts overridden) according the needs in each design screen. But when we generated new library (with html-sketchapp-cli) to for example get updated button colors into use in Sketch library, the buttons in design files lost the edited texts and they were replaced with default texts defined in the library. I think we debugged this and noticed that the random id for symbols was the problem here.

But good news, the #52 seems to fix this update problem (when setting static id in symbolMasterMiddleware), so I think we're good after that!

macintoshhelper commented 5 years ago

Hi, I've opened a PR in html-sketchapp to allow customising do_objectID for library sync. I found that symbol and text object IDs have to stay consistent for library updates.

/cc @kdzwinel

iamstarkov commented 5 years ago

@markdalgleish can you take a look at #67?

mathieudutour commented 5 years ago

@mnikkane the reason why Sketch is not showing the "update library" is because the changeIdentifier is always set to 0: https://github.com/brainly/html-sketchapp/blob/master/html2asketch/model/symbolMaster.js#L107. I'd recommend setting a integer generated from the Date instead.

That's what we do in react-sketchapp:

// Keep track on previous numbers that are generated
let previousNumber = 1;

// Will always produce a unique Number (Int) based on of the current date
function generateIdNumber() {
  let date = Date.now();

  if (date <= previousNumber) {
    previousNumber += 1;
    date = previousNumber;
  } else {
    previousNumber = date;
  }

  return date;
}
iamstarkov commented 5 years ago

@mathieudutour hmm, sounds interesting