console icon indicating copy to clipboard operation
console copied to clipboard

feat: path changes visible and palette

Open nishaaannnt opened this issue 1 year ago • 4 comments

:mag: Overview

FIXES - #304 and #194

:bulb: Proposed Changes

For #304 -

  1. No Extra library used
  2. HeadlessUI + CustomCSS
  3. Functionality completed as mentioned

For #194

  1. GraphQl changes
  2. UI changes

:framed_picture: Screenshots or Demo

Path Fix - Screenshot 2024-07-28 at 6 58 27 PM

Command Palette (Slow due to 8gb ram :) )

https://github.com/user-attachments/assets/8ca47f37-7437-4c41-8d7c-10ab20370933

:memo: Release Notes

New Feature -

  1. Comman palette

Fix -

  1. Path Changes now visible in History

:question: Open Questions

N/A

:test_tube: Testing

Tested everything from my side. Navigation was slow in development but was fast when cached.

:dart: Reviewer Focus

Check directly when you enter console. Press command+k or Control+k

Make changes in path through API and verify the path changes

:heavy_plus_sign: Additional Context

The video used for reference is mentioned in the issue for command palette. UI was done under guidance of @rohan-chaturvedi

:sparkles: How to Test the Changes Locally

Setup is same as contributing.md (No major package changes)

:green_heart: Did You...

  • [x] Ensure linting passes (code style checks)?
  • [x] Update dependencies and lockfiles (if required)
  • [x] Regenerate graphql schema and types (if required)
  • [x] Verify the app builds locally?
  • [x] Manually test the changes on different browsers/devices?

nishaaannnt avatar Jul 28 '24 14:07 nishaaannnt

@nishaaannnt Great work! Could you please restore the 2 files you've deleted in backend/ee/license ? Also since you haven't added any packages, what are the changes to the lockfiles for? Can you revert those if not required?

rohan-chaturvedi avatar Jul 28 '24 15:07 rohan-chaturvedi

@rohan-chaturvedi reverted the lock files. but there is a bug. If i restore the 2 files, the license gets deleted and vice versa. thats pretty annoying and cannot find any solution to it

nishaaannnt avatar Jul 28 '24 15:07 nishaaannnt

@rohan-chaturvedi reverted the lock files. but there is a bug. If i restore the 2 files, the license gets deleted and vice versa. thats pretty annoying and cannot find any solution to it

Could you copy paste them from main and add them back manually?

rohan-chaturvedi avatar Jul 28 '24 15:07 rohan-chaturvedi

@rohan-chaturvedi that is not the issue. it is automatically deleting. even if i manually try it. trying some other ways now

nishaaannnt avatar Jul 28 '24 15:07 nishaaannnt

@nishaaannnt Thanks a lot for taking a crack at this! I like that you managed to put together an implementation for the palette without any external dependencies, and given that we didn't give you enough detail on the required design and functionality, its pretty good!. I'm closing this PR though, for a few reasons:

  1. This PR includes changes to address 2 different unrelated issues in a single PR. This is makes the review more tricky, blocks the fixes for one issue from being merged by the other, and is generally avoidable. Always create separate PRs per issue / feature

  2. The palette code includes the hardcoded open-integrations org in multiple places, and doesn't handle the active organization dynamically, or the ability to switch organization workspaces

  3. The "Create App" and "Invite Member" actions are not particularly useful. Ideally these should have opened the relevant modals.

  4. We were not exhaustive enough in our issue description with the spec for the UI as well as the list of commands required. This is our mistake, but we needed a more fully functional palette with several actions that aren't included here.

rohan-chaturvedi avatar Aug 24 '24 10:08 rohan-chaturvedi