Skip to content

SortableList - add driver and test #2568

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 2 commits into from
Apr 19, 2023

Conversation

M-i-k-e-l
Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l commented Apr 18, 2023

Description

SortableList - add driver and test
WOAUILIB-3444

Changelog

SortableList - add driver and test

Copy link
Contributor

@lidord-wix lidord-wix left a comment

Choose a reason for hiding this comment

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

Looks good overall, I wrote some small comments, and:

  1. Why does the changelog "None"?
  2. Do we need to add some mocks in private or mobile-mocks? (will it potentially break something?)
  3. Consider creating a private version for the module to check that it answers their needs :)

@M-i-k-e-l M-i-k-e-l requested a review from lidord-wix April 19, 2023 06:16
@M-i-k-e-l
Copy link
Collaborator Author

Looks good overall, I wrote some small comments, and:

  1. Why does the changelog "None"?
  2. Do we need to add some mocks in private or mobile-mocks? (will it potentially break something?)
  3. Consider creating a private version for the module to check that it answers their needs :)

Almost forgot :)

  1. I don't think this is something we should publicise ATM, our drivers are still in "incubator" state AFAIK.
  2. It should not break anything, I am still working on private stuff but it should not block this IMO (this is not used AFAIK).
  3. Yes, hopefully I'll get to that soon, but this is a step in the right direction at least.

@lidord-wix
Copy link
Contributor

Looks good overall, I wrote some small comments, and:

  1. Why does the changelog "None"?
  2. Do we need to add some mocks in private or mobile-mocks? (will it potentially break something?)
  3. Consider creating a private version for the module to check that it answers their needs :)

Almost forgot :)

  1. I don't think this is something we should publicise ATM, our drivers are still in "incubator" state AFAIK.
  2. It should not break anything, I am still working on private stuff but it should not block this IMO (this is not used AFAIK).
  3. Yes, hopefully I'll get to that soon, but this is a step in the right direction at least.
  1. The changelog in the release notes really helps us to locate bugs during the release cycle, I'd add it even if it's not relevant to most of our users...
  2. 👍🏽
  3. 👍🏽

@M-i-k-e-l
Copy link
Collaborator Author

I don't really see this having impact on the cycle, but ok

@M-i-k-e-l M-i-k-e-l merged commit 0d913c7 into master Apr 19, 2023
@M-i-k-e-l M-i-k-e-l deleted the infra/sortable-list-add-driver-and-test branch April 19, 2023 07:42
lidord-wix pushed a commit that referenced this pull request May 15, 2023
* SortableList - add driver and test

* Review fixes
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