887-refactor: Partners refactor #892
Conversation
github-actions
bot
changed the title
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Lighthouse Report:
|
src/widgets/partnered/partnered.tsx
Outdated
| <WidgetTitle size="small">Partnered with</WidgetTitle> | ||
| <div className="partners"> | ||
| <div className="partner-logo-container"> | ||
| <ul className={cx('partners')}> |
There was a problem hiding this comment.
Could we use the structure like in Alumni widget?
There was a problem hiding this comment.
I understand a point of keeping a similar HTML structure in all our widgets
Difference for now:
Alumni
<article>
<section>
<WidgetTitle />
<Paragraph />
<section>
{alumni.map(() => (
<figure>
<Image />
</figure>
))}
</section>
</section>
</article>
Partnered
<section>
<div>
<WidgetTitle />
<ul>
{partners.map(() => (
<li>
<Component />
</li>
))}
</ul>
</div>
</section>
- All containers in widgets have
sectiontag. - I't strange to have
article->section->sectionstructure - lists usually use
ul->litags
Therefore, I suggest to keep partnered html structure in both partnered and alumni widgets
| <li className={cx('partner-logo-container')}> | ||
| <JetBrainsLogo /> | ||
| </li> | ||
| <li className={cx('partner-logo-container')}> | ||
| <AwsLogo /> | ||
| </li> | ||
| <li className={cx('partner-logo-container')}> | ||
| <GithubLogo /> | ||
| </li> |
There was a problem hiding this comment.
Can we put our logos in the array above and then output them in a loop in the component? Even for 3 components it would look better.
There was a problem hiding this comment.
It's a good idea - I also moved the array of logos to constants file
| render(<Partnered />); | ||
|
|
||
| expect(screen.getByText('Partnered with')).toBeInTheDocument(); | ||
|
|
||
| expect(screen.getByTestId('widget-title')).toBeInTheDocument(); |
There was a problem hiding this comment.
Can you look tests for AboutCourse, for example. How we checkin the content.
const title = await screen.findByTestId('widget-title'); expect(title).toBeVisible(); expect(title.textContent).toBe('About the course');
There was a problem hiding this comment.
I rewrote the test for checking the correct content to be similar to AboutCourse one
| render(<Partnered />); | ||
|
|
||
| const logos = screen.getAllByRole('img'); | ||
|
|
||
| expect(logos).toHaveLength(3); | ||
|
|
||
| expect(screen.getByAltText('jetbrains icon')).toHaveAttribute('src', jetbrains.src); | ||
| expect(screen.getByAltText('AWS icon')).toHaveAttribute('src', aws.src); | ||
| expect(screen.getByAltText('github icon')).toHaveAttribute('src', github.src); |
There was a problem hiding this comment.
Can you look test for TrainingProgram. Its a good example for check img property
const image = screen.getByTestId('image'); expect(image).toHaveAttribute('alt', expect.stringContaining('AWS Cloud Developer')); expect(image).toHaveAttribute('src', awsDevImg.src);
There was a problem hiding this comment.
I rewrote the test for checking logos to directly verify alt and src attributes
| expect(screen.getByAltText('jetbrains icon')).toHaveAttribute('src', jetbrains.src); | ||
| expect(screen.getByAltText('AWS icon')).toHaveAttribute('src', aws.src); | ||
| expect(screen.getByAltText('github icon')).toHaveAttribute('src', github.src); |
There was a problem hiding this comment.
Please make an array with src and alt properties for every logo and use it.each syntax for this test. You can see example in other test where we use it.each
|
Lighthouse Report:
|
What type of PR is this? (select all that apply)
Description
Refactor partnered module
Related Tickets & Documents
Screenshots, Recordings
I changed a little bit styles for tablets and mobiles
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?