Redocly / redoc

📘 OpenAPI/Swagger-generated API Reference Documentation
https://redocly.github.io/redoc/
MIT License
23.35k stars 2.29k forks source link

Add deprecated css to clickable property name #2526

Closed keremnalbant closed 5 months ago

keremnalbant commented 5 months ago

What/Why/How?

What: ClickablePropertyName styled component doesn't pass the deprecated css to its child <span class='property-name'> and because of that property name doesn't get line-through style.

Solved it by adding the missing css code.

Reference

-

Tests

No tests needed

Screenshots (optional)

Before:

image

After:

image

Check yourself

keremnalbant commented 5 months ago

Hi @keremnalbant, thank you for the contribution. I am not 100% sure about adding deprecated CSS to non-primitive property. The reason is nested properties. In that case, should we use the same style for them? It can confuse if count of properties is a lot.

Hey @AlexVarchuk thanks for consideration. To me as a user it looked like a bug since it doesn't look consistent with the other deprecated properties.

If we approach the problem with a UI POV, it's not a really good practice to have different stylings for deprecated properties depending on their type.

For nested properties, thanks for raising it up. IMO, I don't think we should apply the same for them, deprecated flag and regarding styling only should be applied at root level.

In the API spec I tested against, It didn't have nested properties, let me see check that case and push another commit for that if needed.

AlexVarchuk commented 5 months ago

@lornajane any thoughts?

AlexVarchuk commented 5 months ago

@keremnalbant Also check tests. Seems, we need to update snapshots.

keremnalbant commented 5 months ago

@AlexVarchuk Current implementation doesn't cover the nested properties, and I think that this way it's better because deprecated stylings should be applied at root level.

image

Also since this is my first contribution I couldn't get what you mean by checking the tests. I already run npm run test, npm run test:update-snapshot scripts in my local before committing.

lornajane commented 5 months ago

I think it makes sense to apply the visual label where the deprecated property is applied, regardless of type.