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

[RFR] Add ability to attach an expansion panel to a datagrid row #2634

Merged
merged 8 commits into from
Dec 11, 2018

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Dec 7, 2018

  • Add expand prop to <Datagrid>
  • Document it
  • Test it

peek 07-12-2018 15-02

@fzaninotto fzaninotto changed the title [WIP] Add ability to attach an expansion panel to a datagrid row [RFR] Add ability to attach an expansion panel to a datagrid row Dec 8, 2018
@fzaninotto fzaninotto added this to the 2.6.0 milestone Dec 8, 2018
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

🔥


describe('expand panel', () => {
it('should show an expand button opening the expand element', () => {
cy.contains('1-10 of 13'); // wait for data
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add an expectation here: .should(el => expect(el).to.exist)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to do an assertion here, just wait. Also, the other tests in this file don't do that either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I used to do that too but @Kmaschta pointed out that this is actually not blocking without assertions

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done some reading.

The Cypress docs says you're wrong:

https://docs.cypress.io/guides/core-concepts/introduction-to-cypress.html#For-instance

Core Concept

All DOM based commands automatically wait for their elements to exist in the DOM.

You never need to write .should('exist') after a DOM based command.

And it repeats it:

https://docs.cypress.io/guides/references/best-practices.html#Unnecessary-wait-for-cy-get

And it repeats it again:

https://docs.cypress.io/guides/core-concepts/introduction-to-cypress.html#Commands-Are-Asynchronous

I only found one person saying what you say:

I am very unsure that you're right. If you can reproduce the contains not waiting before executing the next cypress command behavior, I suggest you open an issue in the Cypress bug tracker, because that's a bug of theirs.

Until then, I'll assume Cypress does what their doc says.

Copy link
Contributor

Choose a reason for hiding this comment

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

I admit I haven't tested it

cy.get('[role="expand"]')
.eq(0)
.click();
cy.get('[role="expand-content"]').should(
Copy link
Contributor

Choose a reason for hiding this comment

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

I avoid magic strings expectations as they prevent tools such as the IDE autocompletion and type checking to do their job. I suggest to use the following form: .should(el => expect(el).to.contain(''))

'contain',
'Curabitur eu odio ullamcorper, pretium sem at, blandit libero. Nulla sodales facilisis libero, eu gravida tellus ultrices nec. In ut gravida mi. Vivamus finibus tortor tempus egestas lacinia. Cras eu arcu nisl. Donec pretium dolor ipsum, eget feugiat urna iaculis ut.'
);
cy.get('.datagrid-body').should(
Copy link
Contributor

Choose a reason for hiding this comment

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

same

cy.get('[role="expand"]')
.eq(1)
.click();
cy.get('[role="expand-content"]').should('have.length', 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

same

});

it('should accept multiple expands', () => {
cy.contains('1-10 of 13'); // wait for data
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing expectation

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above, it's just a wait

@djhi djhi merged commit 523145c into next Dec 11, 2018
@djhi djhi deleted the datagrid-expand branch December 11, 2018 18:05
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

2 participants