Skip to content

fix: removed NameValidations to properly show default values #178

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 30, 2021
Merged

fix: removed NameValidations to properly show default values #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b9530b
fix: removed NameValidations to properly show default values
Nov 29, 2021
fe3e7a9
chore: update snapshot
Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/__tests__/__snapshots__/index.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3583,7 +3583,10 @@ exports[`HTML Output should match tickets.schema.json 1`] = `
</div>
</div>
</div>
<div><span>examples</span></div>
<div>
<span>Example value:</span>
<span>true</span>
</div>
</div>
<div></div>
</div>
Expand Down
23 changes: 1 addition & 22 deletions src/components/shared/Validations.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import capitalize from 'lodash/capitalize.js';
import keys from 'lodash/keys.js';
import omit from 'lodash/omit.js';
import pick from 'lodash/pick.js';
import pickBy from 'lodash/pickBy.js';
import uniq from 'lodash/uniq.js';
import * as React from 'react';

Expand Down Expand Up @@ -108,13 +107,8 @@ function filterOutOasFormatValidations(format: string, values: Dictionary<unknow

export const Validations: React.FunctionComponent<IValidations> = ({ validations, hideExamples }) => {
const numberValidations = pick(validations, numberValidationNames);
const booleanValidations = omit(
Copy link
Contributor

@P0lip P0lip Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

you prob want to revert it and simply add default to excluded validations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that excludes it both from booleanValidations and keyValueValidations but I want them to still show in keyVal ones. But I can fix that. I'm just wondering, Is there any usecase where it makes more sense to use NameValidations rather than KeyValueValidations?

Because, if the value is true or false I think it makes more sense to display it using KeyValueValidations, so it says default: true rather than Default. Same for example or even for exclusiveMinimum (which wont be shown here because it falls in numberValidations)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any usecase where it makes more sense to use NameValidations rather than KeyValueValidations?

That's a good question... I'm not really sure whether we actually have any booleanish validations left now that we filter out default, readOnly, writeOnly, and everything related to numbers is handled separately.

I think it makes more sense to display it using KeyValueValidations, so it says default: true rather than Default

I agree

Copy link

Choose a reason for hiding this comment

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

We used NameValidations before because some of they sounded better this way (the property name was enough) it ie. deprecated, readOnly, writeOnly etc. Default can be any type, but agree that as boolean it might look weird. If it makes more sense to change it given those validations are represented differently now then go ahead, I think it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Yea I think most of them are handled differently now (all those mentioned included)

pickBy(validations, v => ['true', 'false'].includes(String(v))),
excludedValidations,
);
const keyValueValidations = omit(validations, [
...keys(numberValidations),
...keys(booleanValidations),
...excludedValidations,
...(hideExamples ? exampleValidationNames : []),
]);
Expand All @@ -123,7 +117,6 @@ export const Validations: React.FunctionComponent<IValidations> = ({ validations
<>
<NumberValidations validations={numberValidations} />
<KeyValueValidations validations={keyValueValidations} />
<NameValidations validations={booleanValidations} />
</>
);
};
Expand Down Expand Up @@ -176,20 +169,6 @@ const KeyValueValidation = ({ name, values }: { name: string; values: string[] }
);
};

const NameValidations = ({ validations }: { validations: Dictionary<unknown> }) => (
<>
{keys(validations).length ? (
<Flex flexWrap maxW="full">
{keys(validations)
.filter(key => validations[key])
.map(key => (
<Value key={key} name={key} className="sl-text-muted sl-capitalize" />
))}
</Flex>
) : null}
</>
);

const Value = ({ name, className }: { name: string; className?: string }) => (
<Text
px={1}
Expand Down Expand Up @@ -226,7 +205,7 @@ export function getValidationsFromSchema(schemaNode: RegularNode) {
: null),
...('annotations' in schemaNode
? {
...(schemaNode.annotations.default ? { default: schemaNode.annotations.default } : null),
...(schemaNode.annotations.default !== void 0 ? { default: schemaNode.annotations.default } : null),
...(schemaNode.annotations.examples ? { examples: schemaNode.annotations.examples } : null),
}
: null),
Expand Down