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

forms: improve diversity of fields #154

Merged

Conversation

anikachurilova
Copy link
Contributor

@anikachurilova anikachurilova commented Feb 6, 2023

Previews:

  1. End DateTime is None - display empty value

Screenshot 2023-02-06 at 09 53 35

2.1 Render checkbox (Active) and dropdown (Category) fields.
2.2 Message field - text area
2.3 Prefill Start DateTime with default value (utcnow) and Active with True
Screenshot 2023-02-08 at 16 54 48

Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

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

Can you please fix the linter warnings?

Copy link
Member

@alejandromumo alejandromumo left a comment

Choose a reason for hiding this comment

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

Left very minor comments, good job :)

* display empty value if field is None
* add possibility to prefill fields with default value
* add possibility to specify the width of table column (search view)
* add render of checkbox, dropdown and textarea fields
* closes inveniosoftware/invenio-banners#10
Comment on lines +28 to +31
{columns.map(([property, { text, order, width }], index) => {
if (!width) {
width = index === 0 ? undefined : index === 1 ? 4 : 3;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍
Maybe in the future we need to find alternative methods to display the grid, as I had to do a custom back-end bound solution in the pages administration panel for it to look nice.

Copy link
Contributor

@jrcastro2 jrcastro2 left a comment

Choose a reason for hiding this comment

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

LGTM! Just left one minor comment

Comment on lines +62 to +63
options: formFieldConfig?.options || fieldSchema?.metadata?.options,
rows: formFieldConfig?.rows || fieldSchema?.metadata?.rows,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
options: formFieldConfig?.options || fieldSchema?.metadata?.options,
rows: formFieldConfig?.rows || fieldSchema?.metadata?.rows,
options: formFieldConfig?.options,
rows: formFieldConfig?.rows,

Minor: If I am not wrong the fieldSchema is a json representation of the marshmallow schema, this will never have options and rows attributes, unless I am missing something.

Copy link
Contributor Author

@anikachurilova anikachurilova Feb 15, 2023

Choose a reason for hiding this comment

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

You're totally right, marshmallow schema doesn't have rows and options, but it has metadata, to which we can
put anything we want. See here: https://github.com/inveniosoftware/invenio-banners/pull/15/files#diff-97da9b93fd82ecb3e421f77c8f0d82f16edeba13ed1b154cdc44572be3c37a3dR25

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, was confused by the naming, we use metadata as well for the record's metadata, but I don't have better alternatives, thanks for clarifying!

@@ -25,8 +25,11 @@ export const SearchResultsContainer = ({
<Table>
<Table.Header>
<Table.Row>
{columns.map(([property, { text, order }], index) => {
const width = index === 0 ? undefined : index === 1 ? 4 : 3;
{columns.map(([property, { text, order, width }], index) => {
Copy link
Contributor

@jrcastro2 jrcastro2 Feb 15, 2023

Choose a reason for hiding this comment

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

Thanks for this! This solves this comment here for @Pineirin

@ppanero ppanero merged commit 9fc3390 into inveniosoftware:main Feb 15, 2023
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.

Integrate invenio-administration
6 participants