Skip to content

Fixes ui #300

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

Merged
merged 5 commits into from
Mar 13, 2023
Merged

Fixes ui #300

merged 5 commits into from
Mar 13, 2023

Conversation

zorox112-practicum
Copy link
Contributor

@zorox112-practicum zorox112-practicum commented Mar 1, 2023

replace strong to red *
add possible reasons
add ClipboardButton response body

Screenshot 2023-03-01 at 15 22 16

Screenshot 2023-03-01 at 15 22 50

Screenshot 2023-03-01 at 15 26 46

@zorox112-practicum zorox112-practicum marked this pull request as ready for review March 1, 2023 12:28
@@ -228,9 +228,19 @@ function merge(value: OpenJSONSchemaDefinition): OpenJSONSchema {
properties[k] = v;
}
}
return {type: 'object', description, properties};
return {type: 'object', description, properties, allOf: value.allOf};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

allOf use for find ref

@@ -11,7 +11,7 @@ function anchor(ref: string) {
}

export function tableParameterName(key: string, required?: boolean) {
return required ? `<strong>${key}*</strong>` : key;
return required ? `${key}<span style="color: var(--yc-color-text-danger);">*</span>` : key;
Copy link
Member

Choose a reason for hiding this comment

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

i would prefer styles being factored out into css file and span having class name applied here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed:
e9461d1

if (value.enum) {
return 'string';
}
return value.type;
Copy link
Member

Choose a reason for hiding this comment

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

this whole function might as well be:

if (value.enum) {
  return 'string';
}

return value.type

or

return value.enum ?? value.type;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enum has the type "JSONSchema6Type[] | undefined" and it has possible values for enum, not their type. The purpose of the function is to set the string value "string" if the type was not specified in the specification

 export type JSONSchema6Type =
    | string 
    | number
    | boolean
    | JSONSchema6Object
    | JSONSchema6Array
    | null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function should output the value of the "type" field if it is set explicitly, substitute "type: 'string'" if it is an enum and return the original type if it is not an enum, I can write in the last return like this:
return undefined

It seems that in the current version it is clearer that the function will return the original type than hardcoding undefined

Copy link
Member

Choose a reason for hiding this comment

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

This function should output the value of the "type" field if it is set explicitly, substitute "type: 'string'" if it is an enum and return the original type if it is not an enum, I can write in the last return like this: return undefined

It seems that in the current version it is clearer that the function will return the original type than hardcoding undefined

is there the case of:
type being set explicitly(to value other than 'string') and enum being provided for one entity?

Copy link
Contributor Author

@zorox112-practicum zorox112-practicum Mar 3, 2023

Choose a reason for hiding this comment

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

type being set explicitly(to value other than 'string')

yes

enum being provided for one entity

I don't understand this part of the question

Copy link
Member

Choose a reason for hiding this comment

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

type being set explicitly(to value other than 'string')

yes

enum being provided for one entity

I don't understand this part of the question

do we have a case when type is being set to value other that string and enum is also provided at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we have a case when type is being set to value other that string and enum is also provided at the same time

yes.
example#1

        name:
          type: integer
          enum:
            - 1
            - 2

Generated type:

export const PetNameEnum = {
    NUMBER_1: 1,
    NUMBER_2: 2
} as const;

example#2:

        name:
          type: string
          enum:
            - 1
            - 2

Generated type:

export const PetNameEnum = {
    _1: '1',
    _2: '2'
} as const;

In the OPEN API specification, enum is always an array.
https://github.com/OAI/OpenAPI-Specification/blob/main/schemas/v3.0/schema.json#L404

I fixed the type and made the logic, if type was not passed, then type is taken from enum[0]

@zorox112-practicum zorox112-practicum force-pushed the fixes branch 2 times, most recently from 1a886fd to e9461d1 Compare March 6, 2023 12:25
@moki
Copy link
Member

moki commented Mar 7, 2023

@zorox112-practicum i can approve ui changes as they are rn, and we will move on with this PR.

if you want to finish implementation of json6schema normalization, cherry pick commits that are about it onto another separate branch -> make PR then we will continue discussion there.

don't forget to cleanup commit history tho.

@zorox112-practicum
Copy link
Contributor Author

divided into a pool of requests:
#301
#302

here only UI

@zorox112-practicum
Copy link
Contributor Author

@moki ^

@zorox112-practicum
Copy link
Contributor Author

@moki
this is the final version, sorry ^

@moki
Copy link
Member

moki commented Mar 9, 2023

@zorox112-practicum

cleanup commit history:

rebase squash:

f0ec15e into dc051c2
c78258e into 88e2794

give meaningful name to each commit to have clear changelogs

describe functionality that you fix/add concisely in header and optionally go in depth inside the commit body.

commit messages:

f7bde27 i see that you set data attribute for the link element why tho i have no idea, describe what problem you are fixing with it

dc051c2 same goes for the replacing strong with red

6adb8e0 better commit message would be display request failure debug information

@@ -1,7 +1,7 @@
import React from 'react';
import {Text, Card} from '@gravity-ui/uikit';

import {Text as TextEnum, yfmSandbox} from '../../constants';
import {Text as TextEnum, yfmSandbox, possibleReasonsFailToFetch} from '../../constants';
Copy link
Member

Choose a reason for hiding this comment

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

i've missed the point we started declaring constants in a camelCase

you wouldn't have to do as TextEnum if usual convention of having constants in uppercase SNAKE_CASE were followed.

about naming FETCH_FAILURES would be clear enough to describe constant values and more concise.

Evgenii Fedoseev added 4 commits March 9, 2023 10:39
Now on the info and sandbox tabs, the display of the same parameter looks different, for this I bring them to the same view
Right now the error message has a great font and is not informative enough. Make it more understandable.
Now links with blob give an error when trying to download them. data-attribute solves this problem.
@moki moki merged commit 2f89ca4 into master Mar 13, 2023
@moki moki deleted the fixes branch June 1, 2023 07:19
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.

2 participants