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

Develop #3852

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Develop #3852

wants to merge 23 commits into from

Conversation

m-bayrak
Copy link

@m-bayrak m-bayrak commented Dec 7, 2023


  • Icon implemented using background-image CSS property
  • Inputs are written inside of 'form' tag with correctly passed attributes
  • All Typical Mistakes from BEM lesson theory are checked.
  • Code follows all the Code Style Rules ❗️

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

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

looks good.
please check comments and try to fix tests.

<h1>Search bar airbnb</h1>

<form data-qa="big">
<div class="search-bar search-bar--larger" tabindex="1">

Choose a reason for hiding this comment

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

it is redundant to make search-bar--larger modifier for big form. set all styles in search-bar and change necessary in search-bar-mini modifier for small form.
same for other elements.

Copy link
Author

Choose a reason for hiding this comment

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

but in this case the CSS file will become larger because many identical lines will appear

Copy link
Author

Choose a reason for hiding this comment

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

Mant thx for comments


.input:focus {
background: none;
font-weight: 900;

Choose a reason for hiding this comment

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

tru do not set font-weight for focus. looks like such font styles should be for input, and for placeholder set font styles that in design. it can help fix tests.

Copy link
Author

Choose a reason for hiding this comment

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

the bugs are fixed and the test is now working

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

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

Well done.
Tests should passed, but looks like you are not deployed after fixes.
Please update deploy.

@m-bayrak
Copy link
Author

m-bayrak commented Dec 16, 2023

This is where I posted the site and the report

m-bayrak.github.io/layout_search-bar-airbnb/src/
m-bayrak.github.io/layout_search-bar-airbnb/backstop_data/engine_scripts/report/html_report/index.html
because there were errors that I described below ↓↓↓

On the computer localhost:8080, the search page is displayed normally and the test passes. When creating a Pull Request on the GitHub platform, a conflict occurs during verification (passing the test) based on the package.json file prnt.sc/4aJyvflXJzTj

When I fix these errors then the test on GitHub runs without conflicts, but the report is not displayed and the site is displayed without CSS settings prnt.sc/Mo2RvqbCTuH7

So I deployed the site to GitHub in the develop branch and manually uploaded the report.
In different projects, from time to time the Mate script gives similar failures

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

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

Well done

@m-bayrak
Copy link
Author

Well done

many thx

Mate script Bug
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