Skip to content
This repository has been archived by the owner on Feb 21, 2019. It is now read-only.

Support latest version of Slate #83

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

quentez
Copy link

@quentez quentez commented Aug 26, 2018

Still a work in progress.
I need to get all of the tests working.

@omar-sr88
Copy link

@quentez I can also take a look into this whenever you cant work on it, let me know how your workflow is going

@quentez
Copy link
Author

quentez commented Aug 29, 2018

@omar-sr88 I got most things working, and we're actually using this branch right now in our product. I still have issues getting all the tests to pass though. The problem appears to be that the data structure for the selection changed, and because of this it doesn't work to initialize it from input.yaml (or at least not in its current form). Any idea how we could address that?

@Soreine
Copy link
Contributor

Soreine commented Aug 29, 2018

It would be great to convert all these tests to slate-hyperscript. It would make it less coupled with the JSON representation. I was planning to do this using slate-hyperprint through the CLI. I did it for slate-edit-table and others, but not this one sadly...

@omar-sr88
Copy link

Hi there, I started to take a look at moving from yaml files to jsx ones but I am struggling to make the correct version of the yamls into jsx. Not sure if Im getting the structure is correct when I need empty paragraphs specifically

@omar-sr88
Copy link

@Soreine Could you please do one of the tests here to use hyperscript?

I have tried working with backsapace-empty-between-inline first, I have converted
the input.yaml to

    <value>
        <document>
            <ul_list>
                <list_item>
                    <paragraph>
                        <inline type="link">First item</inline>
                    </paragraph>
                </list_item>
                <list_item>
                    <paragraph>
                        <text key="_selection_key" />
                    </paragraph>
                </list_item>
                <list_item>
                    <paragraph>
                        <inline type="link">Third item</inline>
                    </paragraph>
                </list_item>
            </ul_list>
        </document>
    </value>

and the expected.yaml

To:

    <value>
        <document>
            <ul_list>
                <list_item>
                    <paragraph>
                        <inline type="link">First item</inline>
                    </paragraph>
                </list_item>
            </ul_list>
            <paragraph />
            <ul_list>
                <list_item>
                    <paragraph>
                        <inline type="link">Third item</inline>
                    </paragraph>
                </list_item>
            </ul_list>
        </document>
    </value>

Followed the pattern used on slate-edit-table for this, but my result keeps showing me the paragraph inside of a li and not by itself. If I could get some clarification on this I could apply the move on everything.

@Soreine
Copy link
Contributor

Soreine commented Sep 9, 2018

@omar-sr88 I have converted all tests to hyperscript.

I'm sorry, I can't merge this right now. We are still thinking whether we will upgrade our codebase to the new Slate version at GitBook. We might fork Slate for a moment. We will communicate on this precisely this week.

@Nantris
Copy link

Nantris commented Sep 10, 2018

@Soreine can I ask what potential limitations you're facing that are making you consider forking?

@eamonnsullivan
Copy link

Hi, we're actually depending on this change (we upgraded slate and it broke this plugin). Is there an ETA on the decision to merge? Is there something I can do (i.e., remaining tasks) to move things along?

@Nantris
Copy link

Nantris commented Sep 17, 2018

We might fork Slate for a moment. We will communicate on this precisely this week.

@Soreine @SamyPesse do you have any update on your decision?

@SamyPesse
Copy link
Member

@slapbox We've just published a readme on our fork explaining the reasons: https://github.com/GitbookIO/slate/blob/master/Readme.md

@Nantris
Copy link

Nantris commented Sep 18, 2018

@SamyPesse thanks very much for the update. Your reasons make a lot of sense. I know internally upgrading to 0.40.0 before forking probably makes no sense, but I think you'd have the potential to rope in a larger chunk of the community if you upgraded before forking. I don't know how much interest Gitbook has in keeping the community though, as I don't know the strategic goals as well.

Best of luck with this fork and thanks for all you've contributed to the Slate community. You too @Soreine!

@tommoor
Copy link
Contributor

tommoor commented Sep 18, 2018

@SamyPesse thanks for the update – would you be interested in exchanging ownership of the npm module if a fork continues your work tracking the Slate mainline?

@quentez
Copy link
Author

quentez commented Sep 25, 2018

I've fixed all the tests and made the code compatible with the latest Slate version.
I think at this point, we'll have to create a new package for people who want to use the official Slate build (unless ownership can be transferred).

(The lint fails because of bugs in the Gitbook ESLint config).

@quentez
Copy link
Author

quentez commented Sep 27, 2018

Update: I've published that PR here for the time being. I make no guarantee of stability/maintainability for now, but it may be helpful for people who are blocked.

We will probably start a new package soon that we will maintain in the long run, but this probably won't happen for another month.

@Nantris
Copy link

Nantris commented Sep 27, 2018

@quentez thanks very much for your work.

@svenadlung
Copy link

@quentez Thank you very much for your work! I've loaded your package with newest version of Slate and it doesn't work as expected. Did you try it with newest version of Slate?

@cdunn
Copy link

cdunn commented Nov 16, 2018

@svenadlung I started a PR to upgrade to latest slate here: frontapp#1

would be good to get a second set of eyes on it and whether it seems like its working

@svenadlung
Copy link

@cdunn Thx a lot. For me your PR looks pretty good! No problems so far.

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.

9 participants