Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance Jsx #10637

Merged
merged 25 commits into from
Feb 15, 2024
Merged

Enhance Jsx #10637

merged 25 commits into from
Feb 15, 2024

Conversation

WillsterJohnsonAtZenesis
Copy link
Contributor

The Jsx system now has custom elements, slots, and several other useful pieces of functionality. There is also a setup for <qx:* /> special elements to be added in future for any purposes, which reserves the qx: namespace in the meantime.

Custom Elements

Any function or constructor can be used as a custom element so long as it's name begins with an uppercase letter or it's accessed by property index. It also must return an instance of qx.html.Node so that it can be added to the VDOM.

Slots

Dynamic & declarative way of adding children to predefined locations within a custom element's Jsx structure. This is syntax sugar for the new .inject() method available to elements.

CSS Custom Properties

More lightweight applications may benefit from leaning in to using the VDOM directly in places, creating a small collection of Jsx components. As styling can be more easily done via CSS for Jsx expressions, it makes sense to provide a simple api for passing CSS variables. <MyElem __css_var="red"> will apply --css_var: red; to the cascade from MyElem downwards.

docs/core/qxjsx.md Outdated Show resolved Hide resolved
add more detail on the benefit and application of passing css custom properties through via double-underscored Jsx attributes
Reframe the Jsx docs to focus on the SSR capabilities afforded
Copy link
Member

@hkollmann hkollmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests should run

Copy link
Contributor

@cboulanger cboulanger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot comment on the PR since I don't know this part of qooxdoo at all. As Tobias said, it would be great to have some more information how this is used in a qooxdoo app - the PR says something about existing jsx support - I couldn't find anything in the documentation on this?

@WillsterJohnsonAtZenesis
Copy link
Contributor Author

I've just pushed a few changes regarding fixing bugs, passing tests and fleshing out the documentation, but I'll also add some more context here.

the PR says something about existing jsx support - I couldn't find anything in the documentation on this?

@johnspackman added Jsx support in 2021. He tells me he meant to add some docs for it, but must've forgotten for some reason.

could you add some notes on how this ties into a regular qx application

It doesn't. At least not directly.

I briefly mentioned it in the updated SSR docs, and I'll give some more context here.
The benefit we've been getting from Jsx is in the generation of printable documents for reports and invoices. Some of these documents are small, maybe only one page, but some of theme are huge - clocking in at hundreds of pages.

These documents are impossible to get right with Qooxdoo desktop, especially when we want to have a pagination process which flows and fits to the physical paper during printing. Aside from the runtime overhead getting out of hand, the development process itself feels excessive - all we need for this is to put data on a page.

In addition to that, getting the styles and layout to work in print is a lot of work for not a particularly brilliant result. Because Qooxdoo desktop is for applications, print media isn't something it has mature APIs for; nobody needs to print an app.

Using Jsx has solved all of these issues and made the development of these reports significantly faster and simpler.

Here's what this Jsx proposal is not doing; it's not a competitor or alternative to Qooxdoo desktop.
QxJsx has no reactivity at all, something quite crucial for building an application. It might be possible to mess around with bindings, but with how difficult it is to access deeply nested elements in a Jsx expression, that would be considerably more effort than just using Qooxdoo desktop.

The main purpose here is to fill a gap which qooxdoo desktop leaves open, that being quick, simple, and effective generation of printable and/or server-rendered static documents.

@oetiker
Copy link
Member

oetiker commented Dec 12, 2023

@WillsterJohnsonAtZenesis that sounds cool ... so lets have some documentation done, in a way that everyone can make use of the new abilities ... examples please :)

@hkollmann
Copy link
Member

Seems that you break some tests....

TESTTAPPER: Running tests in chromium

Start watching ...
not ok 1277 - qx.test.bom.Basic:testElementAttributes - [31.40 ms] - Expected value to be the CSS color '0,0,0' (rgb(0,0,0)), but found value 'rgba(0, 0, 0, 0)' (rgb(-1,-1,-1))!
not ok 1280 - qx.test.bom.Blocker:testBlockElement - [5.80 ms] - Expected '199' but found 'auto'!
not ok 1281 - qx.test.bom.Blocker:testBlockerColor - [5.70 ms] - Expected '#ff0000' but found 'rgba(0, 0, 0, 0)'!
not ok 1447 - qx.test.bom.element.Style:testSetFloat - [12.40 ms] - Expected 'left' but found ''undefined''!
not ok 2887 - qx.test.mobile.core.Widget:testEnabled - [9.50 ms] - Expected 'none' but found 'auto'!
not ok 2890 - qx.test.mobile.dialog.Menu:testSetListHeight - [37.00 ms] - Expected '10800' but found '7200'!
not ok 2891 - qx.test.mobile.dialog.Menu:testMaxListHeight - [55.80 ms] - Expected '375' but found '792'!

@johnspackman
Copy link
Member

My app with this PR is broken. Got the next error:

Looks like this PR is not BC atm.

Could you provide an example?

@WillsterJohnsonAtZenesis
Copy link
Contributor Author

My app with this PR is broken.

I can't find a way to reproduce this error, would you be able to provide a reproduction/steps to reproduce?
That error will only occur if an element is told to use a dom node after it has already acquired one. As far as I'm able to tell the loadNode function shouldn't be attempting to do that. If it is, then that certainly needs fixing.

@goldim
Copy link
Contributor

goldim commented Dec 18, 2023

I am not sure that I can because app is very big. I used to create own custom HTML elements based widgets. Maybe this code could be sent here?

@johnspackman
Copy link
Member

I am not sure that I can because app is very big. I used to create own custom HTML elements based widgets. Maybe this code could be sent here?

If you could put a snippet in that would be great - the issue is where custom DOM is then being loaded into the VDOM methods (ie the opposite of the serialize method)

@goldim
Copy link
Contributor

goldim commented Dec 18, 2023

@johnspackman I'll try. Usually I work with SVG and create DOM for it and put it into virtual dom. I think this case.

@goldim
Copy link
Contributor

goldim commented Dec 19, 2023

@WillsterJohnsonAtZenesis Here you are:

qx.Class.define("Widget",
{
    extend : qx.ui.core.Widget,

    members :
    {
        /**
         * Overwritten from qx.ui.core.Widget.
         */
        _createContentElement : function(){
            const el = new qx.html.Element;
            const str = `<svg width="100%" height="100%"><circle cx="50%" cy="50%" r="50%" stroke="black" stroke-width="3" fill="red" /></svg>`;
            el.useMarkup(str);
            return el;
        }
    }
});

var doc = this.getRoot();
doc.add(new Widget(500, 500),
{
  left : 100,
  top  : 50
  
});

@hkollmann
Copy link
Member

What's the state of this MR?

@goldim
Copy link
Contributor

goldim commented Jan 26, 2024

What's the state of this MR?

I wait for author response on the bug which I found.

@WillsterJohnsonAtZenesis could you check my case above please? It doesn't work.

@johnspackman
Copy link
Member

What's the state of this MR?

I wait for author response on the bug which I found.

@WillsterJohnsonAtZenesis could you check my case above please? It doesn't work.

We couldnt reproduce it, but i came across it today - i've written a fix but need to do some testing first

@goldim
Copy link
Contributor

goldim commented Jan 27, 2024

@johnspackman I tried and reproduced the error once again.
Steps:

  1. Download WillsterJohnsonAtZenesis:jsx-enhance
  2. npm ci and bootstrap-compiler
  3. create some test project npx your\path\to\qooxdoo\bootstrap\qx create test --type=desktop --out=test -I
  4. add my code to project (separate class and use it from app class for example)
  5. run npx your\path\to\qooxdoo\bootstrap\qx serve -S
  6. open page with app and if you don't see anything then u got an error in console

It would be great if somebody of reviewers tries it too to confirm the error.
OS on which I tested is Windows 10 and Node v18.16.0.

@johnspackman
Copy link
Member

That's because I've written the fix but not added it to the PR yet :).

Literally came across it late yesterday afternoon and got my use case working but want to run the unit tests etc before committing to the PR

@WillsterJohnsonAtZenesis
Copy link
Contributor Author

@WillsterJohnsonAtZenesis Here you are:

[...]

@goldim I've checked your demo with the new changes this morning and it seems to be working correctly

@goldim
Copy link
Contributor

goldim commented Feb 14, 2024

@WillsterJohnsonAtZenesis Here you are:
[...]

@goldim I've checked your demo with the new changes this morning and it seems to be working correctly

Thank you, I will see it today-tomorrow.

@goldim
Copy link
Contributor

goldim commented Feb 14, 2024

@WillsterJohnsonAtZenesis Yes, it fixed my problem. Thank you.
I don't use jsx in my qx apps and would like to know if this is a proper way to do it in qx:

qx.Class.define("Widget",
{
    extend : qx.ui.core.Widget,

    members :    {
        _createContentElement : function(){
            return <><h1>aaa</h1><h2>bbb</h2></>;
        }
    }
});

Just I get the next error:

Widget.js:1485 Uncaught TypeError: el.connectObject is not a function
    at wrapper.__createContentElement__P_21_1 (Widget.js:1485:10)
    at wrapper.construct (Widget.js:57:34)
    at wrapper [as constructor] (Class.js:1848:39)
    at wrapper.defaultConstructor (Class.js:1800:33)
    at new wrapper (Class.js:1848:39)
    at wrapper.main (Application.js:51:15)
    at Object.ready (BaseInit.js:77:28)
    at Direct.js:133:37
    at Function._true [as then] (Utils.js:149:22)
    at Direct.js:132:26
    at Array.forEach (<anonymous>)
    at wrapper.dispatchEvent (Direct.js:107:19)
    at wrapper.wrappedFunction [as dispatchEvent] (Interface.js:529:31)
    at wrapper.dispatchEvent (Manager.js:958:22)
    at Registration.js:361:40
    at Function._true [as then] (Utils.js:149:22)

but if it is:

 return <div><h1>aaa</h1><h2>bbb</h2></div>;

everything is fine.

@WillsterJohnsonAtZenesis
Copy link
Contributor Author

@WillsterJohnsonAtZenesis Yes, it fixed my problem. Thank you. I don't use jsx in my qx apps and would like to know if this is a proper way to do it in qx:

The second option looks more correct to me.

A fragment, ie the tags without a tagname, are just syntax sugar for creating an array. Eg, myJsx and myArray below are equivalent;

const myJsx = (
  <>
    <p>Foo bar</p>
    <p>Hello world</p>
  </>
);

const myArray = [
  <p>Foo bar</p>
  <p>Hello world</p>
];

With this comparison hopefully you can see why the error el.connectObject is not a function appears, as el in this case would be an array.

I think we're actually using qx.data.Array rather than a native Array for fragments, but the effect is the same.

The new 'SSR' and 'Using Jsx' docs pages provide a little more info on using Jsx to compliment a desktop app, though using Jsx directly within a Desktop app by overriding _createContentElement is also valid.

Copy link
Contributor

@goldim goldim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillsterJohnsonAtZenesis I got you, thank you. Didn't know about implementation <></>. Also would be great to add svg for jsx too. Now svg element doesn't appear, at least there is no error.

@hkollmann hkollmann merged commit 364b7c4 into qooxdoo:master Feb 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants