X Tutup
The Wayback Machine - https://web.archive.org/web/20200814035417/https://github.com/nodejs/nodejs.dev/pull/197
Skip to content

/ nodejs.dev

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

refactor: Simplify logic to mark navigation items as done #197

Merged
merged 7 commits into from Mar 14, 2019
Merged

refactor: Simplify logic to mark navigation items as done #197

merged 7 commits into from Mar 14, 2019

Conversation

@sagirk
Copy link
Member

sagirk commented Mar 13, 2019 •
edited

Description

  • Unify logic to mark navigation items as done
  • Lighten the data structure, tracking only the unique slugs of already read sections
  • Use a native set (Set<NavigationSectionItem['slug']>) for tracking the above
  • Rename the data structure semantically

Related Issues

Refs: #187
sagirk added 4 commits Mar 13, 2019
@sagirk
refactor: Mark navigation items as done
ba57780 
- Lighten the data structure by excluding unnecessary navigation data
- Use native maps
- Use a semantic name for the DS

Refs: #187
@sagirk
refactor: Make isRead flag simpler
48c05e6
@sagirk
fix: Change `ref` to `innerRef` in <Link> to pacify TS
85508f5
@sagirk
refactor: Simplify further
Loading status checks…
ab5c8ec
@sagirk sagirk self-assigned this Mar 13, 2019
@sagirk sagirk added the refactor label Mar 13, 2019
@BeniCheni
BeniCheni approved these changes Mar 13, 2019
View changes
Copy link
Contributor

BeniCheni left a comment

LGTM with minor questions.
src/components/navigation.tsx Outdated Show resolved Hide resolved
src/components/navigation-item.tsx Outdated Show resolved Hide resolved
src/components/navigation-section.tsx Outdated Show resolved Hide resolved
sagirk added 2 commits Mar 13, 2019
@sagirk
fix: Add type; upgrade packages; revert innerRef
Loading status checks…
e118c95
@sagirk
refactor: Use set instead of map to make the DS even lighter
Loading status checks…
7efe5a0
@LaRuaNa
Copy link
Contributor

LaRuaNa commented Mar 14, 2019

/gcbrun
@github-actions
Copy link

github-actions bot commented Mar 14, 2019

Preview at: https://storage.googleapis.com/staging.nodejs.dev/7efe5a0/index.html
@LaRuaNa
LaRuaNa reviewed Mar 14, 2019
View changes
Copy link
Contributor

LaRuaNa left a comment

LGTM 👍 Just see a lot of changes made here e118c95 . Did you deleted yarn.lock and hit yarn install or why that many changes? Would be better if we remove only the packages we don't need because I see a lot of upgrades which might lead to misbehaviour and it will be quite hard to understand what it causes.
@sagirk
fix: Revert package upgrades
Loading status checks…
37e2ec0
@sagirk
Copy link
Member Author

sagirk commented Mar 14, 2019

@LaRuaNa Reverted all the package upgrades. We can upgrade as needed on separate PRs later.
@LaRuaNa
LaRuaNa approved these changes Mar 14, 2019
View changes
Copy link
Contributor

LaRuaNa left a comment

lovely 👍
@sagirk sagirk merged commit 21298b6 into master Mar 14, 2019
2 checks passed
2 checks passed
Post Staging Link Post Staging Link
Details
Travis CI - Pull Request Build Passed
Details
@sagirk sagirk deleted the refactor/mark-nav-done-logic branch Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LaRuaNa LaRuaNa

@BeniCheni BeniCheni

Assignees

@sagirk sagirk

Labels
refactor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@sagirk @LaRuaNa @BeniCheni
You can’t perform that action at this time.
X Tutup