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

feat: remove last updated alert from homepage #604

Merged
merged 6 commits into from
Aug 9, 2021
Merged

feat: remove last updated alert from homepage #604

merged 6 commits into from
Aug 9, 2021

Conversation

antoooks
Copy link
Contributor

@antoooks antoooks commented Aug 9, 2021

Closes #598

Description

Remove "last updated" alert from homepage

Current Tasks

  • Remove the component rendering from the home page
  • Remove the widget from Netlify CMS
  • Update the failing tests accordingly

- remove LastUpdatedAlert component from homepage
- remove widget from netlify-cms config
- fix testing
@netlify
Copy link

netlify bot commented Aug 9, 2021

✔️ Deploy Preview for wargabantuwarga ready!

🔨 Explore the source changes: 3a3319b

🔍 Inspect the deploy log: https://app.netlify.com/sites/wargabantuwarga/deploys/6110e924ec09e7000814003c

😎 Browse the preview: https://deploy-preview-604--wargabantuwarga.netlify.app

@antoooks antoooks marked this pull request as ready for review August 9, 2021 05:08
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #604 (3a3319b) into main (66a9013) will increase coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #604      +/-   ##
==========================================
+ Coverage   79.57%   79.69%   +0.12%     
==========================================
  Files         110      110              
  Lines        1268     1266       -2     
  Branches      418      418              
==========================================
  Hits         1009     1009              
+ Misses        253      251       -2     
  Partials        6        6              
Impacted Files Coverage Δ
pages/index.tsx 88.88% <ø> (-11.12%) ⬇️
components/ui/alert/utils/helpers.ts 100.00% <0.00%> (+2.04%) ⬆️
components/ui/alert/alert.tsx 100.00% <0.00%> (+14.28%) ⬆️

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 66a9013...3a3319b. Read the comment docs.

ryanadhi added a commit to ryanadhi/wargabantuwarga.com that referenced this pull request Aug 9, 2021
@ryanadhi ryanadhi mentioned this pull request Aug 9, 2021
3 tasks
Copy link
Member

@mazipan mazipan 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 to me

But the codecov still failed.

Help to fix it first, Mostly on components/ui/alert/**

@mazipan mazipan added the automerge To be merged automatically once all the requirements are fulfilled label Aug 9, 2021
- improve code coverage on components/ui/alert/**
@kodiakhq kodiakhq bot merged commit d7e93dd into kawalcovid19:main Aug 9, 2021
@antoooks antoooks deleted the feature/remove-last-updated branch August 9, 2021 08:45
Copy link
Member

@zainfathoni zainfathoni left a comment

Choose a reason for hiding this comment

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

We shouldn't decrease the test coverage of any well-covered code. 😅

I'm reopening #598 issue to restore the test properly.

Comment on lines -10 to -21
describe("HomePage", () => {
it("renders the last updated time correctly", () => {
render(<HomePage latestNews={[latestNewsItemBuilder()]} />);

expect(
screen.getByText(
/Pembaruan terakhir pada Selasa, 27 Juli 2021 17.43 WIB/i,
),
).toBeVisible();
});
});

Copy link
Member

@zainfathoni zainfathoni Aug 9, 2021

Choose a reason for hiding this comment

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

Thanks for working on it, @antoooks.
Hiwever, this test removal reduced the code coverage of the home page. Please put it back and change the assertion instead.

FYI, Mas @mazipan & @resir014.

Copy link
Member

@zainfathoni zainfathoni left a comment

Choose a reason for hiding this comment

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

I have more requests to fix the way we're testing the components here.

@@ -45,5 +52,12 @@ describe("Alert", () => {
"border-red-400",
);
expect(container.firstChild?.firstChild).toHaveClass("p-4", "babayaga");
expect(
container.firstChild?.firstChild?.firstChild?.firstChild?.firstChild,
Copy link
Member

Choose a reason for hiding this comment

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

container.firstChild?.firstChild?.firstChild?.firstChild?.firstChild,
).toHaveClass("flex-shrink-0 mr-3");
expect(
container.firstChild?.firstChild?.firstChild?.childNodes[1],
Copy link
Member

Choose a reason for hiding this comment

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

const { container } = render(<HomePageContent />);

it("renders correctly", () => {
expect(container.firstChild).toHaveClass("flex-1");
Copy link
Member

Choose a reason for hiding this comment

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

Please perform more meaningful assertions that's obvious to the user experience other than merely asserting this flex-1 class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge To be merged automatically once all the requirements are fulfilled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the "last updated" alert on the home page
4 participants