Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
    Joshua Denzau
    @joshdenz
    The things I suggested were the things that were required to actually run the tests in my environment. After cloning the repo and installing dependencies running npm test resulted in no tests being found. I needed to explicitly specify the __tests__ folder in package.json and I needed to use escaped double quotes for the path else no tests were found.
    My apologies if everything was running smoothly in your environment.
    Rishi Kumar Chawda
    @rishichawda
    Mm.. seems weird. you can change it then. which env are you on, by the way?
    Rishi Kumar Chawda
    @rishichawda
    And also, checkout from development branch.
    Joshua Denzau
    @joshdenz
    I'm on windows 10
    Rishi Kumar Chawda
    @rishichawda
    okay.
    Did you start writing tests?
    I'm working on more components.
    Joshua Denzau
    @joshdenz
    I have started working on tests. I have started with the button component. But now, it is time for bed. Have a nice day!
    Joshua Denzau
    @joshdenz
    oh @rishichawda regarding the render.test.js accessibility.test.js edgecase.test.js convention, I don't see a problem with that. That seems like an appropriate split to make.
    Rishi Kumar Chawda
    @rishichawda
    Cool. we'll go with that then.
    @KRRISH96 has started working on the library too and we're also looking for more developers online to start contributing. Hoping to see the activity increase in the repository.
    Joshua Denzau
    @joshdenz
    It's odd that the test structure didn't work for me as you had initially written it, when it worked for you.
    Rishi Kumar Chawda
    @rishichawda
    Yeah. Works in macOS and linux. I haven't used windows before.
    Joshua Denzau
    @joshdenz
    I'm fairly certain that the double quotes issue in package.json is caused by Windows being Windows lol
    Joshua Denzau
    @joshdenz
    Actually I think this github issue describes the problem I am seeing mochajs/mocha#2138
    Rishi Kumar Chawda
    @rishichawda
    Ah. This explains why the original command wasn't able to find the test files.
    Rishi Kumar Chawda
    @rishichawda
    @joshdenz How's it going?
    Joshua Denzau
    @joshdenz
    @rishichawda to be honest I haven't looked at any code in a couple of days as my wife and daughter have the flu and I have been taking care of them.
    Rishi Kumar Chawda
    @rishichawda
    @joshdenz Oh, take care guys.
    Joshua Denzau
    @joshdenz
    Thanks. It has been a trying time. Everyone is getting better though slowly. Thanks for the kind words.
    Joshua Denzau
    @joshdenz
    I've had a chance to look at this again and I'm running into an issue with the Button component. If you render the button with type="light" the color should be rendered as color.dark but instead you get the defaultProp color.light
    Rishi Kumar Chawda
    @rishichawda
    the color in the button's context actually refers to the text color. As it is the case with html element - color refers to the text color. For button's color, there is a separate prop named bg.
    Do the prop names look confusing? @joshdenz @KRRISH96
    Joshua Denzau
    @joshdenz
    no no, I know that... what I mean is that if you render the button with type="light" it should render the color as color.dark... meaning the text color should be dark to contrast the light bg color
    but instead the text color is rendered as color.light
    so the button ends up white with white text
    Rishi Kumar Chawda
    @rishichawda
    Oh. Let me see.
    Rishi Kumar Chawda
    @rishichawda
    @joshdenz I've fixed that. will raise a pr and merge it on development.
    You can pull latest development branch and work on it.
    Joshua Denzau
    @joshdenz
    what was the issue, Im curious?
    Rishi Kumar Chawda
    @rishichawda
    was a typo in the code. had it wrapped in a parenthesis 😅
    Joshua Denzau
    @joshdenz
    haha, of course it was a typo facepalm
    Rishi Kumar Chawda
    @rishichawda
    You can pull development now. I've raised the PR and merged. this is the commit with fix : 550f4d4
    sorry about being so late. was on a call
    Joshua Denzau
    @joshdenz
    its ok, I'm home today with no deadlines so I'm very relaxed :)
    Rishi Kumar Chawda
    @rishichawda
    great! how's your SO and baby? are they fine now?
    Joshua Denzau
    @joshdenz
    yes, everyone is healthy again. Out of 30 kids in my daughters class, last week 20 of them including 2/3rds of the teachers were sick
    pneumonia, flu, fever... its been a mess
    but thankfully my girls are doing well
    Rishi Kumar Chawda
    @rishichawda
    Good to hear. God bless you guys. Take care.
    Joshua Denzau
    @joshdenz
    thanks again for the kind words
    how often would you like pull requests for these tests? As in do you want all tests, render, accessibility, edgecase to be completed for a component before a pull request? What did you have in mind?
    Rishi Kumar Chawda
    @rishichawda
    We could have tests for each component in a separate PR. It will simplify the flow.
    Joshua Denzau
    @joshdenz
    so instead of a PR for each test type, just do one PR for each component once all the tests are done?
    Rishi Kumar Chawda
    @rishichawda
    Which means, we can add accessibility, render and edgecase tests to the same PR. this will help in ensuring that a PR is dedicated to a particular component and we can ensure the coverage for the component in the same PR itself before merging

    so instead of a PR for each test type, just do one PR for each component once all the tests are done?

    Yes.

    Joshua Denzau
    @joshdenz
    understood
    Rishi Kumar Chawda
    @rishichawda
    Merged development today so that master branch has nyc version pinned to 13.1.0( it was pinned to that version on development ) so that greenkeeper bot doesn't open any issues again for nyc updates breaking the tests.
    Now we'll merge development only when sufficient progress has been made. Most probably for the first milestone.