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

fix: closing isolated vue #1627

Merged
merged 11 commits into from
Jul 22, 2020
Merged

fix: closing isolated vue #1627

merged 11 commits into from
Jul 22, 2020

Conversation

elevatebart
Copy link
Collaborator

fixes #1479

Remark For technical reasons, I had to remove the return from isolate button here: I could not access href without going through a context or a prop train.

It looked logical to me: If there was only one action it should not be in two places.
With this change, it would be only on the top right. Tell me if you need a fix for it.

Screen Shot 2020-07-11 at 11 20 36 PM

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks, this is a lot of great stuff! I've left couple of minor comments and questions.

@@ -26,49 +26,46 @@ exports[`should filter list when search field contains a query 1`] = `
searchTerm=""
Copy link
Member

Choose a reason for hiding this comment

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

Let's get rid of these snashots — they aren't really testing anything ;-/ We used to have way too many but there's no reason to keep them ;-)

https://blog.sapegin.me/all/snapshot-tests/

src/client/utils/processComponents.ts Outdated Show resolved Hide resolved
@sapegin
Copy link
Member

sapegin commented Jul 14, 2020

I had to remove the return from isolate button here

I think it's fine ;-)

@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #1627 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
...mponents/ComponentsList/ComponentsListRenderer.tsx 100.00% <ø> (ø)
...t/rsg-components/SectionHeading/SectionHeading.tsx 100.00% <ø> (ø)
...mponents/SectionHeading/SectionHeadingRenderer.tsx 100.00% <ø> (ø)
...t/rsg-components/ComponentsList/ComponentsList.tsx 100.00% <100.00%> (ø)
...t/rsg-components/ReactComponent/ReactComponent.tsx 91.66% <100.00%> (ø)
src/client/rsg-components/Sections/Sections.tsx 100.00% <100.00%> (ø)
...rsg-components/TableOfContents/TableOfContents.tsx 100.00% <100.00%> (+2.22%) ⬆️
src/client/rsg-components/slots/IsolateButton.tsx 100.00% <100.00%> (ø)
src/client/utils/getUrl.ts 92.85% <100.00%> (ø)
src/client/utils/processComponents.ts 91.66% <100.00%> (+4.16%) ⬆️
... and 4 more

@elevatebart
Copy link
Collaborator Author

Hello @sapegin, I hope you are well.
Should I merge this one?

@sapegin
Copy link
Member

sapegin commented Jul 21, 2020

The code is good now but I still see changes in a gigantic snapshot that I'd love to remove entirely ;-)

@elevatebart
Copy link
Collaborator Author

The code is good now but I still see changes in a gigantic snapshot that I'd love to remove entirely ;-)

Snapshots replaced with real RTL tests. I hope you like them.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

This is much better! Couple of suggestions.

expect(getAllByTestId('rsg-toc-link').length).toBe(6);
fireEvent.change(getByPlaceholderText('Filter by name'), { target: { value: searchTerm } });
expect(getAllByTestId('rsg-toc-link').length).toBe(1);
expect(getByTestId('rsg-toc-link').innerHTML).toBe('Forms');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(getByTestId('rsg-toc-link').innerHTML).toBe('Forms');
expect(getByTestId('rsg-toc-link')).toHaveTextContent('Forms');

expect(actual).toMatchSnapshot();
expect(getAllByTestId('rsg-toc-link').length).toBe(6);
fireEvent.change(getByPlaceholderText('Filter by name'), { target: { value: searchTerm } });
expect(getAllByTestId('rsg-toc-link').length).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(getAllByTestId('rsg-toc-link').length).toBe(1);
expect(getAllByTestId('rsg-toc-link')). toHaveLength(1);

);

expect(actual).toMatchSnapshot();
expect(getAllByTestId('rsg-toc-link').length).toBe(6);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(getAllByTestId('rsg-toc-link').length).toBe(6);
expect(getAllByTestId('rsg-toc-link')).toHaveLength(6);

@elevatebart
Copy link
Collaborator Author

elevatebart commented Jul 21, 2020

Agreed! Updated a few matchers

@sapegin sapegin merged commit 9b6b6f6 into master Jul 22, 2020
@sapegin sapegin deleted the come-back-from-fullscreen branch July 22, 2020 06:22
@sapegin
Copy link
Member

sapegin commented Jul 22, 2020

Yay, merging! 🦄

@styleguidist-bot
Copy link
Collaborator

🎉 This PR is included in version 11.0.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Closing isolated view doesn't return to correct location when using sections
3 participants