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

feat: add modelcollapse to primitive models #7557

Merged

Conversation

twjasa
Copy link
Contributor

@twjasa twjasa commented Oct 14, 2021

Add collapse to primitive models

Description

Now primitive-model is wrapped inside a ModelCollapse component to let the user expand and collapse all the content of each primitive models such as (string, number, integer and boolean)

Motivation and Context

This is a fix to the following issue number #7549

How Has This Been Tested?

Screenshots (if appropriate):

Screen.Recording.2021-10-14.at.6.04.23.PM.mov

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@mathis-m
Copy link
Contributor

@twjasa can you have a look at the failing tests please? I'll review this initially tomorrow and when tests are green! Thanks for your work so far :)

@mathis-m
Copy link
Contributor

@twjasa looks like the test is failing because the click is called on multiple elements in the features/model-collapse.js tests:

  • Swagger 2.0: Model should collapse and expand when toggled clicking button
  • Swagger 3.0: Model should collapse and expand when toggled clicking button

@twjasa
Copy link
Contributor Author

twjasa commented Oct 14, 2021

Hey @mathis-m thanks for the suggestion, will look into it in a few hours.

@twjasa
Copy link
Contributor Author

twjasa commented Oct 15, 2021

I just commited new changes to the behavior of collapse in primitivemodel. Now all tests passed 😄 . Never worked with cypress, it's amazing what it can do.

@mathis-m
Copy link
Contributor

@twjasa nice thanks for your effort, I will have a look at this later today.
If you are interested in frontend testing frameworks for your own projects have a look at https://github.com/microsoft/playwright, it's the new kid on the marked and has grown fast, it almost has the same star count on GitHub.

@twjasa
Copy link
Contributor Author

twjasa commented Oct 18, 2021

@twjasa nice thanks for your effort, I will have a look at this later today. If you are interested in frontend testing frameworks for your own projects have a look at https://github.com/microsoft/playwright, it's the new kid on the marked and has grown fast, it almost has the same star count on GitHub.

Thanks! I will definitely see it.

Copy link
Contributor

@tim-lai tim-lai left a comment

Choose a reason for hiding this comment

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

@twjasa Thanks for the PR! In reviewing the proposed changes, I don't think that the existing unit tests should be removed. At a glance, it looks like you would just need to add a prop to expandDepth to force the component to display as expanded, so that you can test for elements inside.

@tim-lai tim-lai self-assigned this Mar 10, 2022
@twjasa
Copy link
Contributor Author

twjasa commented Mar 11, 2022

Hey @tim-lai thanks for the suggestion, you were right about expandDepth and I recover back those tests with some changes to make it work with ModelCollapse.

Please take a look whenever you can. Thanks again.

@twjasa twjasa requested a review from tim-lai March 11, 2022 18:39
@tim-lai tim-lai merged commit 77d0bb9 into swagger-api:master Mar 21, 2022
@tim-lai
Copy link
Contributor

tim-lai commented Mar 21, 2022

@twjasa PR merged! Thanks for the update and contribution!

@gcardoso22
Copy link

This seems to have caused #8008

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

4 participants