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: Add static slug for province detail page #361

Merged
merged 16 commits into from
Jul 25, 2021

Conversation

Namchee
Copy link
Contributor

@Namchee Namchee commented Jul 23, 2021

Closes #288

Description

Generate static slugs for all provinces and apply all related changes to the client side code.

Current Tasks

  • Generate slug from the etc/fetcher, slugify the title, e.g: DKI Jakarta will be dki-jakarta
  • Use the slug on the client side, without any additional index
  • Test the functionality

@netlify
Copy link

netlify bot commented Jul 23, 2021

✔️ Deploy Preview for wargabantuwarga ready!

🔨 Explore the source changes: 5fe61d8

🔍 Inspect the deploy log: https://app.netlify.com/sites/wargabantuwarga/deploys/60fd79f351069000070fa1d9

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

@Namchee
Copy link
Contributor Author

Namchee commented Jul 24, 2021

This PR is waiting for #313 to be resolved first, as slug generation depends on getKebabCase

@zainfathoni
Copy link
Member

This PR is waiting for #313 to be resolved first, as slug generation depends on getKebabCase

#313 has been merged, @Namchee, so you can continue working on this PR.
Thanks!

@codecov
Copy link

codecov bot commented Jul 25, 2021

Codecov Report

Merging #361 (5fe61d8) into main (cf1637d) will increase coverage by 1.62%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #361      +/-   ##
==========================================
+ Coverage   25.34%   26.96%   +1.62%     
==========================================
  Files          66       66              
  Lines         864      864              
  Branches      253      252       -1     
==========================================
+ Hits          219      233      +14     
+ Misses        641      627      -14     
  Partials        4        4              
Impacted Files Coverage Δ
etc/fetchers/fetch-sheets.ts 0.00% <0.00%> (ø)
pages/provinces/[provinceSlug]/index.tsx 0.00% <0.00%> (ø)
lib/provinces.ts 16.66% <25.00%> (+16.66%) ⬆️
pages/provinces/index.tsx 57.14% <100.00%> (+57.14%) ⬆️
lib/string-utils.ts 50.00% <0.00%> (+5.55%) ⬆️
lib/date-utils.ts 60.00% <0.00%> (+60.00%) ⬆️

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 cf1637d...5fe61d8. Read the comment docs.

@Namchee Namchee marked this pull request as ready for review July 25, 2021 03:19
@zainfathoni
Copy link
Member

Please fix the CI checks, @Namchee, thanks! 🙏

@Namchee
Copy link
Contributor Author

Namchee commented Jul 25, 2021

@zainfathoni the netlify build will always fail since I added a new static property slug when generating data with etc/fetchers. The new format isn't generated when netlify executes yarn build which will cause provinceSlug to be undefined thus causing next build to throw errors (as it expects the slug to be at least null or empty string).

Can we make sure that the app fetches fresh data before any build?

@zainfathoni
Copy link
Member

@zainfathoni the netlify build will always fail since I added a new static property slug when generating data with etc/fetchers. The new format isn't generated when netlify executes yarn build which will cause provinceSlug to be undefined thus causing next build to throw errors (as it expects the slug to be at least null or empty string).

Can we make sure that the app fetches fresh data before any build?

You can temporarily revert this line into using the fetch-wbw script again, so that we can merge this PR to the main branch.
Once it has been merged to the main branch, the mirror-box script gets the latest version of your fetch-wbw.js implementation after a few minutes. After that, you can raise another PR to revert your changes to use the mirror-box again. 😅

I know it's a hassle, but I don't think we need to optimize this workflow, because #374 is coming soon anyway, and it will render both fetch-wbw and mirror-box useless. 😅

@Namchee
Copy link
Contributor Author

Namchee commented Jul 25, 2021

Side note: The test is really minimal, just to pass codecov. I'll put other tests if we have guidelines for it.

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.

LGTM 💯

Thanks for working on it, @Namchee! 🙏

@zainfathoni
Copy link
Member

I just created a new issue #412 to revert back the one-line change you made to get this PR merged, @Namchee. 😁

@zainfathoni
Copy link
Member

This PR is waiting for #313 to be resolved first, as slug generation depends on getKebabCase

Once my #394 PR is merged, you can work on it to close the #413 ticket. 😁

@zainfathoni zainfathoni merged commit ebb6ee3 into kawalcovid19:main Jul 25, 2021
@Namchee Namchee mentioned this pull request Jul 25, 2021
1 task
@mazipan mazipan mentioned this pull request Jul 25, 2021
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.

Static slug for province detail page
3 participants