Skip to content

Conversation

rohit2sharma95
Copy link

@rohit2sharma95 rohit2sharma95 commented Nov 19, 2020

Ref: twbs#32123 (comment)

Though, this PR is missing some unit tests for data.js 😄

@rohit2sharma95
Copy link
Author

Now seems good? @alpadev 🙂

@alpadev
Copy link
Owner

alpadev commented Nov 20, 2020

About the missing unit tests. Do you think we need to add some more?

@rohit2sharma95
Copy link
Author

Yes, because I just removed some unit tests (Just to pass tests) instead of updating them.
So definitely those tests should be added 🙂

@rohit2sharma95
Copy link
Author

Who will do work on those tests 😄
Just asking so that we both do not work for the same

@alpadev
Copy link
Owner

alpadev commented Nov 20, 2020

As you like.

@rohit2sharma95
Copy link
Author

Okay I will do then 👍

@alpadev
Copy link
Owner

alpadev commented Nov 20, 2020

One of the tests you removed wasn't properly done anyway 😁

I think some of them are of no use anymore because you removed the key/bsKey but 'should remove data if something is stored' could be readded. Thats like a complete workflow 👍

@rohit2sharma95
Copy link
Author

Okay, then I think we should move this PR to the main repository first 😄

@alpadev alpadev merged commit ba4728e into alpadev:dom-data-map Nov 20, 2020
@rohit2sharma95 rohit2sharma95 deleted the dom-data-map branch November 20, 2020 16:30
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