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

OEL-780: Fix assert contains selectors #139

Merged
merged 19 commits into from
Jan 6, 2022
Merged

OEL-780: Fix assert contains selectors #139

merged 19 commits into from
Jan 6, 2022

Conversation

escuriola
Copy link
Contributor

@@ -1,51 +1,51 @@
form assertion:
render:
markup:
'#markup': Hello World!
'#markup': "Hello World!"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make a bit of order for quotes. Let's use single quotes when needed, like for multiple words (even if it's not strictly needed). Use double quotes only when we need special characters like \n .

'#type': textarea
'#title': Textarea
'#value': Text
'#type': "textarea"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we quoted all of these?

@@ -27,7 +27,8 @@ accordion:
'div.accordion-item:nth-child(1) button.accordion-button': "Accordion Item #1"
'div.accordion-item:nth-child(2) button.accordion-button': "Accordion Item #2"
'div.accordion-item:nth-child(3) button.accordion-button': "Accordion Item #3"

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this for consistency.

- "Bullfights. Bull hockey"
- "Advice on living, working or travelling in the EU"
- "Lorem ipsum dolor sit amet"
'div.accordion-item:nth-child(1) div.accordion-body': "Bullfights. Bull hockey"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to the code, but let's not speak about animal violence here :) can you swap this text with a less "crude" one?

Comment on lines 33 to 34
'div.accordion-item:nth-child(2) div.accordion-body': "Advice on living, working or travelling in the EU"
'div.accordion-item:nth-child(3) div.accordion-body': "Lorem ipsum dolor sit amet"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change the 3 contains assertions with equals assertions, and check the full text is shown.

@@ -137,4 +137,4 @@ navbar_medium_dark_background_color_dark_with_disable_collapse:
'label': "Search"
'a.navbar-brand': "OpenEuropa Site"
contains:
'a': "https://europa.eu/"
'nav div.container': "https://europa.eu/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@@ -80,4 +80,4 @@ offcanvas_with_trigger_with_icon:
'div.offcanvas-body': "This is the offcanvas body content."
contains:
'button[data-bs-toggle="offcanvas"]': "Toggle offcanvas"
'icon': 'bootstrap-icons.svg#filter'
'svg': 'bootstrap-icons.svg#filter'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as others.

Comment on lines +61 to +62
'table > thead > tr td:nth-child(4)': '<b>Bold header</b>'
'table > tbody > tr:nth-child(3) td:nth-child(3)': '<b>Bold cell</b>'
Copy link
Contributor

Choose a reason for hiding this comment

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

Uniform with the ones above? Or uniform all of them to this one :)

contains:
'thead th:nth-child(1)': '<b>Bold header</b>'
'tbody tr:nth-child(3) td:nth-child(3)': '<b>Bold cell</b>'
'table > thead > tr td:nth-child(4)': '<b>Bold header</b>'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

contains:
'thead th:nth-child(1)': '<b>Bold header</b>'
'tbody tr:nth-child(3) td:nth-child(3)': '<b>Bold cell</b>'
'table > thead > tr td:nth-child(4)': '<b>Bold header</b>'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor

@brummbar brummbar left a comment

Choose a reason for hiding this comment

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

Some leftovers, didn't look through all of it, it's too much :)
Let's fix these 3 and merge it like this.

Comment on lines 49 to 52
'div.carousel-item img[src="https://images.site.example1"]': 1
'div.carousel-item img[src="https://images.site.example2"]': 1
'div.carousel-item img[alt="alt image 1"]': 1
'div.carousel-item img[alt="alt image 2"]': 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we add :nth-child(N) too, so we check that images are actually in the correct order?

'img[src="https://images.site.example2"]': 1
'img[alt="alt image 1"]': 1
'img[alt="alt image 2"]': 1
'div.carousel-item img[src="https://images.site.example1"]': 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

contains:
'span__day': "26"
'span.pb-3.text-uppercase': 'May'
'span[class="bg-light w-100 text-center py-2 text-dark rounded-bottom mb-n1"]': '2021'
Copy link
Contributor

Choose a reason for hiding this comment

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

why [ instead of .

brummbar
brummbar previously approved these changes Jan 4, 2022
icon_name: "alarm"
heading: 'Well done!'
message: 'A simple alert. check it out!'
icons_path: '/assets/icons/bootstrap-icons.svg'
Copy link
Contributor

@drishu drishu Jan 5, 2022

Choose a reason for hiding this comment

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

pls create a follow up ticket that sets this inside the pattern template and doesn't expose it as a parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'#fields':
settings:
'hero': false
'shade': false
'full_width': true
'centered': true
'title': "EU Budget for the future"
'description': "Innovation, economy, environment and geopolitics"
'title': 'EU Budget for the future'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'title': 'EU Budget for the future'
title: 'EU Budget for the future'

we should use quotes for keys only when it would be invalid yml if not, like '#something' or 'div.something'
same for the others below

'#type': 'pattern'
'#id': 'button'
'#fields':
'settings':
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'settings':
settings:

@drishu drishu merged commit 091197c into 1.x Jan 6, 2022
@drishu drishu deleted the OEL-780 branch January 6, 2022 08:50
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

3 participants