Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Remove unnecessary outdated dependencies #892

Merged
merged 2 commits into from
Mar 13, 2019

Conversation

midori0507
Copy link

  • Remove react-document-title, use react-helmet
  • Remove react-body-classname, classname append can be handled by classnames

test('renders DocumentTitle component', () => {
const title = getWrapper().find(DocumentTitle);
expect(title.prop('title')).toBe('Test title - Varaamo');
test('renders Helmet title', () => {
Copy link
Author

Choose a reason for hiding this comment

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

as Helmet is not easily render a component which have prop, have to use mount instead of shallow rendering.

The code below did the same thing needed to test

@midori0507 midori0507 self-assigned this Mar 12, 2019
@codecov-io
Copy link

codecov-io commented Mar 13, 2019

Codecov Report

Merging #892 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #892   +/-   ##
========================================
  Coverage    91.51%   91.51%           
========================================
  Files          217      217           
  Lines         2828     2828           
  Branches       498      498           
========================================
  Hits          2588     2588           
  Misses         212      212           
  Partials        28       28
Impacted Files Coverage Δ
app/pages/AppContainer.js 94.73% <ø> (ø) ⬆️
app/pages/PageWrapper.js 100% <ø> (ø) ⬆️
app/shared/favicon/Favicon.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fce22e9...ce74a7d. Read the comment docs.

@midori0507 midori0507 force-pushed the feature/remove-react-title-use-helmet branch from 436c1a9 to ce74a7d Compare March 13, 2019 08:37
</div>
</DocumentTitle>
</BodyClassName>
<div className={classNames('app', getCustomizationClassName())}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you 100% sure this is enough, because if there are any styles that depend on body class name then this most likely doesn't work?

Copy link
Contributor

Choose a reason for hiding this comment

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

We just have to be 100% sure that Vantaa/Espoo site styles work like they use to work.

Copy link
Author

@midori0507 midori0507 Mar 13, 2019

Choose a reason for hiding this comment

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

@rBoost: I tested it under Espoo/Vantaa varaamo.vantaa.fi, remove the class from body and move the classname to app, everything still work after that

This is only for css usage

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, good. 👍

@midori0507 midori0507 merged commit af031b0 into develop Mar 13, 2019
@midori0507 midori0507 deleted the feature/remove-react-title-use-helmet branch March 13, 2019 09:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants