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

Add ensa version to sap systems overview entries #1561

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Jun 21, 2023

Description

Last episode in the ensa version series!
At the field to the SAP systems overview page.

image
The screenshot is not taken directly using main branch. It has the deregistration branch rebased.

I didn't add storybook stories this time, as the components have multiple selectors usage in different levels.
It could be fixed, but I thought that this was not the correct time.
I have played a bit using a mocked redux store in storybook, but even though it works, it is pretty horrible, as it takes away all of the options to customize the incoming arguments

How was this tested?

I have added some tests that are not related to this addition, as the view was almost untested.

@arbulu89 arbulu89 added the enhancement New feature or request label Jun 21, 2023
@arbulu89 arbulu89 marked this pull request as ready for review June 21, 2023 15:05
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM, just one nitpick


renderWithRouter(StatefulSapSystemList);

const rows = screen.getByRole('table').querySelectorAll('tbody > tr');
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach is fine but you could also just do a query for 'ENSA1', 'ENSA2' and count the number of nodes that appear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as well.
I wanted to have a more detailed test, where I check the cell position within the table together with the content

Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

Thank you! --> LGTM :D

@arbulu89 arbulu89 merged commit f63e660 into main Jun 26, 2023
@arbulu89 arbulu89 deleted the add-ensa-version-sap-systems-overview branch June 26, 2023 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants