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

Add deprecated css to clickable property name #2526

Merged

Conversation

keremnalbant
Copy link
Contributor

@keremnalbant keremnalbant commented Apr 24, 2024

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

  • Code is linted
  • Tested
  • All new/updated code is covered with tests

@keremnalbant keremnalbant requested a review from a team as a code owner April 24, 2024 17:34
Copy link
Collaborator

@AlexVarchuk AlexVarchuk left a comment

Choose a reason for hiding this comment

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

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.

@keremnalbant
Copy link
Contributor Author

keremnalbant commented Apr 25, 2024

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
Copy link
Collaborator

@lornajane any thoughts?

@AlexVarchuk
Copy link
Collaborator

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

@keremnalbant
Copy link
Contributor Author

@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
Copy link
Contributor

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

@AlexVarchuk AlexVarchuk merged commit b0d03d0 into Redocly:main Apr 25, 2024
3 checks passed
@AlexVarchuk AlexVarchuk mentioned this pull request Apr 25, 2024
3 tasks
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.

None yet

3 participants