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

Recipes drag and drop feature #314

Merged
merged 8 commits into from
Jul 31, 2023
Merged

Recipes drag and drop feature #314

merged 8 commits into from
Jul 31, 2023

Conversation

deepak2431
Copy link
Contributor

@deepak2431 deepak2431 commented Jul 19, 2023

This PR closes #230.

Test plan

Tested it manually.
Run the extension, head over to the recipes section and try to drag and reorder the recipes.

REC-20230719184916.mp4

@deepak2431 deepak2431 marked this pull request as draft July 19, 2023 08:40
Copy link
Contributor

@st0nebreaker st0nebreaker left a comment

Choose a reason for hiding this comment

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

This is great, thank you!!

  • For persisting the indices, I believe you can use the LocalStorageProvider.ts (Memento docs)
  • Is it possible to add some CSS to the dragging item? It'd be great if it had a top white border like the VS Code extension icons do to help the eye track where the items moving too (especially since the tags are full of text).
Screen.Recording.2023-07-20.at.3.56.47.PM.mov

@deepak2431
Copy link
Contributor Author

@st0nebraker Thanks for the review. I will make the CSS changes you suggested.
Also, I will try to use the LocatStorage of VSCode for storing the indices because I thought it couldn't be used with the webviews.

@deepak2431
Copy link
Contributor Author

@st0nebraker I have made all the requested changes for the pre-defined recipes, and also added the state to save the order of it. I think it's all good for review now.

REC-20230723210224.mp4

@deepak2431 deepak2431 marked this pull request as ready for review July 23, 2023 15:42
@deepak2431
Copy link
Contributor Author

@st0nebraker Can I have a review, please?

Copy link
Contributor

@st0nebreaker st0nebreaker 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, thank you!! Just some minor typo, indentation & cleaner conditional css with classNames() suggestions

Comment on lines 15 to 16
border-top: 2px solid var(--vscode-activityBar-activeBorder
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
border-top: 2px solid var(--vscode-activityBar-activeBorder
);
border-top: 2px solid var(--vscode-activityBar-activeBorder);

Comment on lines 145 to 147
className={`${styles.recipeButton} ${
draggedIndex === index ? styles.recipeButtonDrag : ''
}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
className={`${styles.recipeButton} ${
draggedIndex === index ? styles.recipeButtonDrag : ''
}`}
className={classNames(
styles.recipeButton,
draggedIndex === index && styles.recipeButtonDrag
)}

@@ -1,3 +1,5 @@
import { useState } from 'react'

import { VSCodeButton } from '@vscode/webview-ui-toolkit/react'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { VSCodeButton } from '@vscode/webview-ui-toolkit/react'
import { VSCodeButton } from '@vscode/webview-ui-toolkit/react'
import classNames from 'classnames'

Comment on lines 36 to 37
const initalState = vscodeAPI.getState() as State | undefined
const reorderedRecipeList: RecipeListType = initalState?.reorderedRecipes ?? recipesList
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const initalState = vscodeAPI.getState() as State | undefined
const reorderedRecipeList: RecipeListType = initalState?.reorderedRecipes ?? recipesList
const initialState = vscodeAPI.getState() as State | undefined
const reorderedRecipeList: RecipeListType = initialState?.reorderedRecipes ?? recipesList

@deepak2431
Copy link
Contributor Author

@st0nebraker Made all the changes. It's all good to merge now.

Copy link
Contributor

@st0nebreaker st0nebreaker left a comment

Choose a reason for hiding this comment

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

LGTM!

@deepak2431
Copy link
Contributor Author

@st0nebraker Can you merge this PR, please?

@st0nebreaker st0nebreaker merged commit 101f2e2 into sourcegraph:main Jul 31, 2023
8 checks passed
@deepak2431 deepak2431 deleted the deepak/recipes-drag_drop branch November 14, 2023 06:24
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.

Recipes: Drag & drop, user can customize recipe order based on preference
2 participants