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

Side Drawer #221

Merged
merged 20 commits into from
Dec 31, 2019
Merged

Side Drawer #221

merged 20 commits into from
Dec 31, 2019

Conversation

shresthabijay
Copy link
Contributor

@shresthabijay shresthabijay commented Dec 18, 2019

Fixes #180

@shresthabijay shresthabijay marked this pull request as ready for review December 19, 2019 17:03
@shresthabijay
Copy link
Contributor Author

added hamburger menu for both mobile and desktop view and added .npmrc file for setting up bit registry.

@raineorshine raineorshine changed the title integrated very basic material ui drawer for mobile view Side Drawer Dec 19, 2019
@raineorshine
Copy link
Contributor

I recommend rebasing on dev to avoid additional merge conflict headaches.

@shresthabijay
Copy link
Contributor Author

I recommend rebasing on dev to avoid additional merge conflict headaches.

Will do that

Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

I'm very happy with what I see! The functionality is nearly perfect, and you accounted for the many, spread out changes needed to sync with state, local, and remote.

The suggestions are a combination of code quality recommendations, small bugs, and the use of mutations in existingThoughtChange.

Please run the linter manually (npm run lint) as the automated check is not failing properly.

The only other bug not listed here is the neglect of existingThoughtMove. When a thought is moved (either through drag-and-drop or the keyboard command Ctrl + Shift + Up/Down), it breaks recently edited functionality. The links should be updated as with edit.

src/App.css Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/util/sortByLastUpdated.js Outdated Show resolved Hide resolved
src/util/sync.js Outdated Show resolved Hide resolved
src/reducers/existingThoughtDelete.js Outdated Show resolved Hide resolved
src/util/isDescendant.js Outdated Show resolved Hide resolved
src/util/syncRemote.js Outdated Show resolved Hide resolved
@shresthabijay
Copy link
Contributor Author

I'm very happy with what I see! The functionality is nearly perfect, and you accounted for the many, spread out changes needed to sync with state, local, and remote.

The suggestions are a combination of code quality recommendations, small bugs, and the use of mutations in existingThoughtChange.

Please run the linter manually (npm run lint) as the automated check is not failing properly.

The only other bug not listed here is the neglect of existingThoughtMove. When a thought is moved (either through drag-and-drop or the keyboard command Ctrl + Shift + Up/Down), it breaks recently edited functionality. The links should be updated as with edit.

@shresthabijay
Copy link
Contributor Author

I'm very happy with what I see! The functionality is nearly perfect, and you accounted for the many, spread out changes needed to sync with state, local, and remote.

The suggestions are a combination of code quality recommendations, small bugs, and the use of mutations in existingThoughtChange.

Please run the linter manually (npm run lint) as the automated check is not failing properly.

The only other bug not listed here is the neglect of existingThoughtMove. When a thought is moved (either through drag-and-drop or the keyboard command Ctrl + Shift + Up/Down), it breaks recently edited functionality. The links should be updated as with edit.

onExistingThoughtMove should the lastUpdated timestamp for that thought should be updated as well?

Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Thanks!

A small change needs to be made to ensure this works when editing thoughts in the Context View (Ctrl + Shift + S). When in the context view, the path needs to be linearized into a simple context. pathToContext assumes the given path has already been simplified and doesn't handle this unfortunately.

e.g. Given the following structure:

  • A
    • x (cursor)
      • 1
  • B
    • x
      • 2

If the cursor is at A • ~x (where ~ indicates the Context View is activated), then the user can navigate to A • ~x • B and B's child A • ~x • B • 2:

  • A
    • ~x
      • B
        • 2 (cursor)

In this case, the cursor is A • ~x • B • 2, but the recently edited thought should be B • 2.

This should be as simple as using thoughtsNew instead of cursorNew in existingThoughtChange etc.

Let me know if that doesn't make sense! Thanks!

src/components/AppComponent.js Outdated Show resolved Hide resolved
src/components/HamburgerMenu.js Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/reducers/existingThoughtChange.js Outdated Show resolved Hide resolved
src/reducers/existingThoughtDelete.js Outdated Show resolved Hide resolved
src/reducers/existingThoughtMove.js Outdated Show resolved Hide resolved
src/util/syncRemote.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@shresthabijay
Copy link
Contributor Author

Thanks!

A small change needs to be made to ensure this works when editing thoughts in the Context View (Ctrl + Shift + S). When in the context view, the path needs to be linearized into a simple context. pathToContext assumes the given path has already been simplified and doesn't handle this unfortunately.

e.g. Given the following structure:

  • A

    • x (cursor)

      • 1
  • B

    • x

      • 2

If the cursor is at A • ~x (where ~ indicates the Context View is activated), then the user can navigate to A • ~x • B and B's child A • ~x • B • 2:

  • A

    • ~x

      • B

        • 2 (cursor)

In this case, the cursor is A • ~x • B • 2, but the recently edited thought should be B • 2.

This should be as simple as using thoughtsNew instead of cursorNew in existingThoughtChange etc.

Let me know if that doesn't make sense! Thanks!

thoughtsNew is a context while cursorNew is path. So we need to convert the thoughtsNew to a path. Is there any utility function that can do that in the current codebase?

@raineorshine
Copy link
Contributor

thoughtsNew is a context while cursorNew is path. So we need to convert the thoughtsNew to a path. Is there any utility function that can do that in the current codebase?

rankThoughtsFirstMatch

src/components/Sidebar.js Outdated Show resolved Hide resolved
src/reducers/existingThoughtMove.js Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/components/Sidebar.js Outdated Show resolved Hide resolved
src/App.css Outdated Show resolved Hide resolved
src/components/Breadcrumbs.js Outdated Show resolved Hide resolved
src/components/Link.js Outdated Show resolved Hide resolved
src/components/Breadcrumbs.js Outdated Show resolved Hide resolved
Comment on lines 12 to 14
const toggleEllipsis = () => {
setEllipsize(!ellipsize)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this internal to the Breadcrumbs component? I think it makes more sense there than in the Sidebar, which shouldn't be concerned with ellipses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it makes total sense.

@raineorshine raineorshine merged commit 52ebf0d into cybersemics:dev Dec 31, 2019
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.

Recently Edited Side Drawer
2 participants