-
Notifications
You must be signed in to change notification settings - Fork 373
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
layout improvement:replacement of tag fieldset with div/header section #1990
Conversation
I also added a error section. But the property 'errors' is possible missing. I am not sure how to add this. This is just a suggestion Signed-off-by: Ralph Soika <ralph.soika@imixs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
Signed-off-by: Ralph Soika <ralph.soika@imixs.com>
Hi @sdirix have you seen that I improved my pull request and followed your suggestions? |
Yes! Thanks ;) Will take a look soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, however when running the example application (npm run dev
in packages/vanilla
) one can see that the CSS was not yet accounted for, i.e. the label and add button are rendered without any styling at the beginning of the div.
Suggestion:
- Align the table renderer and array control renderer until the
header
element. At the moment they are very similar but not quite.
i.e. copy the following lines from the table renderer
<div className={controlClass} hidden={!visible}>
<header>
<label className={labelClass}>{label}</label>
<button
className={buttonClass}
onClick={addItem(path, createDefaultValue(schema))}
>
Add to {labelObject.text}
</button>
</header>
<div className={divClassNames}>
{!isValid ? errors : ''}
</div>
and then just adjust them by using array.control
instead of array.table
in all places.
Then adjust styles.ts
by adding array.control
entries (you start by copying the array table entries)
Then adjust the example.css
so that it correctly applies to the classes you defined in styles.ts
.
The goal is to have a sensible rendering of the new array.control in the React Vanilla example application.
Sorry, I was not yet able to run a test. I can successful build all packages with $ yarn But when I try to run
Can you help me how to build and test correctly? |
You need to install the base dependencies, then run lerna and then build all packages. Only then can the example application be executed. So starting from the root directory: # reset and clean everything
git reset --hard
git clean -dfx
# build JSON Forms
npm ci
npm run init
npm run build
# Start React Vanilla example application
(cd packages/vanilla && npm run dev) You should not use |
Thanks for this help. Now it works. Which is the issue you are using to test the array in the react test panel? |
Signed-off-by: Ralph Soika <ralph.soika@imixs.com>
README.md
Outdated
If you want to contribute to the project or implement a new renderer components it is recommended to first create a fork of this project so you can later start pull a request to contribute your code. | ||
|
||
For a first time setup: | ||
|
||
* Install [node.js](https://nodejs.org/) (only Node 14 and npm 6 is currently supported) | ||
* Fork this repository on Github | ||
* Clone this repository | ||
* Install dependencies: `npm ci` | ||
* Hook up dependencies between packages: `npm run init` | ||
|
||
Now you can build the project and run the examples. First of all you need to install the base dependencies, then run lerna and then build all packages. After that you can execute the example application. | ||
|
||
So starting from the root directory: | ||
|
||
# reset and clean everything | ||
git reset --hard | ||
git clean -dfx | ||
|
||
# build JSON Forms | ||
npm ci | ||
npm run init | ||
npm run build | ||
|
||
# e.g. Start React Vanilla example application | ||
cd packages/vanilla && npm run dev | ||
|
||
Run the example from your web browser: localhost:3000#person | ||
|
||
<img src="example-01.png" /> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be frank, I don't think we need this and it rather "pollutes" the root Readme. The Readme already covers the first time setup, the build and how to start the example applications. We don't need to explain how git and Github works within the JSON Forms Readme ;)
The only thing missing in the root Readme is on which port the example applications can be found. Note that it's port 8080
and not 3000
as here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can either 'pollute' the readme or you can explain to stupid people like me how to build the project. As a JakartaEE developer I do not have the NodeJS expert knowledge as you expect...
I will remove the changes from the README again and continue working on the CSS fixes....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rsoika, I'm pretty sure that you're not stupid at all, just unfamiliar with the tools used here. The root readme is just the wrong place for this kind of information / writing style. It would rather fit into a contribution guide or in the wiki. If you agree I would copy this over into the developer wiki.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can for example use "Nested Array" ( |
Signed-off-by: Ralph Soika <ralph.soika@imixs.com>
Now I think the layout should be fine. I added the class definitions to styles.ts and also into the example.css I am also now more familiar with the css-class concept - somehow strange ;-) |
Hi, @sdirix, what do you think about the latest update. I hope this is now fine and can be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the contribution! ❤️
I also added a error section. But the property 'errors' is possible
missing. I am not sure how to add this. This is just a suggestion
Signed-off-by: Ralph Soika ralph.soika@imixs.com