-
Notifications
You must be signed in to change notification settings - Fork 15
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] : Added dynamic initial values in create modal #23
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is alot of thing's that make no sense, try to make a PR that is clear and not bloated it, Ideally a PR should not have move then 200-300 line of code.
src/commands/CommandUtility.ts
Outdated
IModify, | ||
IPersistence, | ||
IRead, | ||
IHttp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these spaces
src/commands/CommandUtility.ts
Outdated
public persis: IPersistence; | ||
public triggerId?: string; | ||
public threadId?: string; | ||
public app: QuickRepliesApp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
src/commands/CommandUtility.ts
Outdated
const subCommand = this.params[0].toLowerCase(); | ||
|
||
if ( | ||
subCommand === CommandParam.CREATE || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we creating team in all the command ? explain me this
src/commands/CommandUtility.ts
Outdated
await this.handleSingleParam(handler); | ||
break; | ||
} | ||
default: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be in default
src/commands/CommandUtility.ts
Outdated
} | ||
} | ||
} | ||
private async handleQuickCreate(handler: Handler, args: string[]): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already handled default cases
); | ||
|
||
if (args.length < 3) { | ||
await sendUsageMessage(modify, room, sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this ?
src/handlers/Handler.ts
Outdated
return; | ||
} | ||
} | ||
public app: QuickRepliesApp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too much bloated code remove all these spaces
room: IRoom, | ||
user: IUser, | ||
): Promise<void> { | ||
const text = '/quick-create <replyName> "<replyBody>"'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we have this code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make requested changes
9612a78
to
db79d2c
Compare
Hi @VipinDevelops so i reflected the changes you asked for and made them as per the requirements Note |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still a lot of room for improvement basically you are repeating a lot of code and not following best practice.
src/commands/CommandUtility.ts
Outdated
@@ -77,7 +77,13 @@ export class CommandUtility implements ICommandUtility { | |||
break; | |||
} | |||
default: { | |||
await handler.sendDefault(); | |||
const subCommand = this.params[0].toLowerCase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't have to put this in switch case you can handle this in global
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no updates here ?
src/commands/CommandUtility.ts
Outdated
@@ -107,4 +113,23 @@ export class CommandUtility implements ICommandUtility { | |||
} | |||
} | |||
} | |||
|
|||
private async handleQuickCreate(handler: Handler, args: string[]): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why dont use the function we already have for creating new replies ?
src/commands/CommandUtility.ts
Outdated
return; | ||
} | ||
|
||
switch (args[0].toLowerCase()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why switch case in a function that is made to create a reply ?
src/commands/CommandUtility.ts
Outdated
await handler.CreateQuickReply(args); | ||
break; | ||
} | ||
default: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the third place where you please .sendDefault() is there any other way to achieve check without repeating this
src/handlers/Handler.ts
Outdated
@@ -206,4 +207,20 @@ export class Handler implements IHandler { | |||
return; | |||
} | |||
} | |||
public async CreateQuickReply(args: string[]): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do have a method already for this
Also, update the documentation accordingly. |
db79d2c
to
7d8d7b0
Compare
hey @VipinDevelops made changes as you asked..! now i believe the code looks much cleaner to be merged... |
Hey @not-meet Can you explain a little about changes you have made and use the code we have already for creating replies I have seen you writing some code again that already exist Either reuse the function we have or make something new that can be use in both old and new code. |
hey!! @VipinDevelops I wrote new logic instead of modifying the existing modal to avoid conflicts. While I understand the concern about calling the handler in the default switch case instead of passing it globally, doing so avoids issues where the global call still triggers the switch case unintentionally. |
Hey! @VipinDevelops i have made the changes as you asked! now i believe the code is much cleaner.... also added an example in readme to help user to get the context of quick create cli command! |
QuickRepliesApp.ts
Outdated
@@ -36,6 +36,7 @@ import { settings } from './src/config/settings'; | |||
export class QuickRepliesApp extends App { | |||
private elementBuilder: ElementBuilder; | |||
private blockBuilder: BlockBuilder; | |||
public params: Array<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this ?
QuickRepliesApp.ts
Outdated
@@ -141,6 +142,7 @@ export class QuickRepliesApp extends App { | |||
http, | |||
persistence, | |||
modify, | |||
this.params, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
src/commands/CommandUtility.ts
Outdated
@@ -77,7 +77,13 @@ export class CommandUtility implements ICommandUtility { | |||
break; | |||
} | |||
default: { | |||
await handler.sendDefault(); | |||
const subCommand = this.params[0].toLowerCase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no updates here ?
src/definition/handlers/IHandler.ts
Outdated
@@ -10,4 +10,5 @@ export interface IHandler extends Omit<ICommandUtilityParams, 'params'> { | |||
|
|||
export type IHanderParams = Omit<ICommandUtilityParams, 'params'> & { | |||
language: Language; | |||
args : string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ?
src/modal/createModal.ts
Outdated
room: IRoom, | ||
language: Language, | ||
): Promise<IUIKitSurfaceViewParam | Error> { | ||
args: string[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add this logic again we can just use the arguments to pass in the existing function
src/commands/CommandUtility.ts
Outdated
@@ -76,7 +85,7 @@ export class CommandUtility implements ICommandUtility { | |||
await this.handleSingleParam(handler); | |||
break; | |||
} | |||
default: { | |||
default: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
src/definition/handlers/IHandler.ts
Outdated
@@ -10,4 +10,5 @@ export interface IHandler extends Omit<ICommandUtilityParams, 'params'> { | |||
|
|||
export type IHanderParams = Omit<ICommandUtilityParams, 'params'> & { | |||
language: Language; | |||
args? : string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why optional ? it will always be there
src/handlers/Handler.ts
Outdated
@@ -64,13 +67,19 @@ export class Handler implements IHandler { | |||
this.modify, | |||
this.room, | |||
this.language, | |||
this.args ?? [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
src/handlers/Handler.ts
Outdated
); | ||
|
||
if (modal instanceof Error) { | ||
this.app.getLogger().error(modal.message); | ||
return; | ||
} | ||
|
||
if (!modal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
src/modal/createModal.ts
Outdated
@@ -25,7 +27,29 @@ export async function CreateReplyModal( | |||
modify: IModify, | |||
room: IRoom, | |||
language: Language, | |||
): Promise<IUIKitSurfaceViewParam | Error> { | |||
args: string[], | |||
): Promise<IUIKitSurfaceViewParam | Error | void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no change here
Again fix PR name , please follow the standard for everything you do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alot of changes required
src/commands/QuickCommand.ts
Outdated
@@ -46,7 +48,6 @@ export class QuickCommand implements ISlashCommand { | |||
app: this.app, | |||
}; | |||
|
|||
const commandUtility = new CommandUtility(commandUtilityParams); | |||
await commandUtility.resolveCommand(); | |||
await new CommandUtility(commandUtilityParams).resolveCommand(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ?
src/modal/createModal.ts
Outdated
@@ -56,6 +59,7 @@ export async function CreateReplyModal( | |||
label: labelReplyBody, | |||
optional: false, | |||
multiline: true, | |||
initialValue: args.length > 2 ? args.slice(2).join(' ') : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think that's how you should do this
QuickRepliesApp.ts
Outdated
constructor(info: IAppInfo, logger: ILogger, accessors: IAppAccessors) { | ||
super(info, logger, accessors); | ||
} | ||
|
||
public setCommandParams(userId: string, params: string[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this ?
QuickRepliesApp.ts
Outdated
@@ -98,12 +109,15 @@ export class QuickRepliesApp extends App { | |||
persistence: IPersistence, | |||
modify: IModify, | |||
) { | |||
const userId = context.getInteractionData().user.id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again what is this ?
QuickRepliesApp.ts
Outdated
@@ -98,12 +109,15 @@ export class QuickRepliesApp extends App { | |||
persistence: IPersistence, | |||
modify: IModify, | |||
) { | |||
const userId = context.getInteractionData().user.id; | |||
const storedParams = this.getCommandParams(userId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you strong params makes no sense to me
constructor( | ||
protected readonly app: QuickRepliesApp, | ||
protected readonly read: IRead, | ||
protected readonly http: IHttp, | ||
protected readonly persistence: IPersistence, | ||
protected readonly modify: IModify, | ||
protected readonly params: ICommandUtilityParams['params'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??
@@ -80,6 +81,7 @@ export class ExecuteViewSubmitHandler { | |||
view, | |||
language, | |||
triggerId, | |||
this.params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
const name = nameStateValue ? nameStateValue.trim() : ''; | ||
const body = bodyStateValue ? bodyStateValue.trim() : ''; | ||
|
||
const argsName = params.length > 1 ? params[1] : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why undefined that should not be possible
|
||
const argsName = params.length > 1 ? params[1] : undefined; | ||
const argsBody = params.length > 2 ? params.slice(2).join(' ') : undefined; | ||
const name = argsName ? argsName : nameStateValue ? nameStateValue.trim() : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ternary operator is doing simple thing in much complex way
src/handlers/Handler.ts
Outdated
@@ -47,6 +48,8 @@ export class Handler implements IHandler { | |||
this.triggerId = params.triggerId; | |||
this.threadId = params.threadId; | |||
this.language = params.language; | |||
this.args = params.args; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
hi! sorry for the delay for now i have changed the pr name and in the recent commit was just trying this approach, I will |
3f46624
to
bf658de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read how I named my intial PR and use similar name I won't allow any PR that is not following good naming and description practice.
README.md
Outdated
@@ -76,6 +76,13 @@ By selecting quick replies instead of typing manually, agents/users can respond | |||
- **`/quick ai`**: Use AI to generate replies | |||
- **`/quick help`**: Get help with Quick Reply | |||
- **`/qs <reply name>`**: Quickly search and send a reply by name | |||
- **`/quick create <name> <message>`**: Create a quick reply directly from the input box with a name and message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put create command together , explain how double quotes works to name a reply
src/commands/CommandUtility.ts
Outdated
@@ -76,7 +85,7 @@ export class CommandUtility implements ICommandUtility { | |||
await this.handleSingleParam(handler); | |||
break; | |||
} | |||
default: { | |||
default: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
view.state?.[CreateModalEnum.REPLY_BODY_BLOCK_ID]?.[ | ||
CreateModalEnum.REPLY_BODY_ACTION_ID | ||
]; | ||
const nameStateValue = view.state && Object.values(view.state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code is so bad adding find don't make sense to me here and again you are not using enums , please find why enum are good and how we are using in this case.
src/handlers/Handler.ts
Outdated
@@ -56,6 +59,8 @@ export class Handler implements IHandler { | |||
} | |||
|
|||
public async CreateReply(): Promise<void> { | |||
const cliName = this.args?.[1] || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we give them better naems like initial name or something ? or can we just make one object that have both values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are close
README.md
Outdated
@@ -76,6 +76,13 @@ By selecting quick replies instead of typing manually, agents/users can respond | |||
- **`/quick ai`**: Use AI to generate replies | |||
- **`/quick help`**: Get help with Quick Reply | |||
- **`/qs <reply name>`**: Quickly search and send a reply by name | |||
- **`/quick create <name> <message>`**: Create a quick reply directly from the input box with a name and message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double quotes ("") ? Explain double quote behavior here
src/commands/CommandUtility.ts
Outdated
@@ -65,8 +65,17 @@ export class CommandUtility implements ICommandUtility { | |||
triggerId: this.triggerId, | |||
threadId: this.threadId, | |||
language, | |||
args: this.params, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not name it params only then ?
src/definition/handlers/IHandler.ts
Outdated
@@ -10,4 +10,5 @@ export interface IHandler extends Omit<ICommandUtilityParams, 'params'> { | |||
|
|||
export type IHanderParams = Omit<ICommandUtilityParams, 'params'> & { | |||
language: Language; | |||
args? : string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why optional ? it will always be there
src/handlers/Handler.ts
Outdated
@@ -35,6 +35,7 @@ export class Handler implements IHandler { | |||
public triggerId?: string; | |||
public threadId?: string; | |||
public language: Language; | |||
public args? : string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
src/handlers/Handler.ts
Outdated
@@ -56,6 +59,8 @@ export class Handler implements IHandler { | |||
} | |||
|
|||
public async CreateReply(): Promise<void> { | |||
const initialReplyName = this.args?.[1] || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial values are optional why OR here ? just don't pass it if its not there
@@ -50,6 +50,7 @@ export function inputElementComponent( | |||
text: label, | |||
element: plainTextInputElement, | |||
optional, | |||
blockId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??
src/modal/createModal.ts
Outdated
@@ -25,7 +25,10 @@ export async function CreateReplyModal( | |||
modify: IModify, | |||
room: IRoom, | |||
language: Language, | |||
initialReplyName: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can make it a object but fine for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you make requested changes also share about what you changed in comment or reply
README.md
Outdated
|
||
#### Example: | ||
```sh | ||
/quick create "greeting" Hello! How have you been? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what I ment
How double quotes make the multiple words a single text for body use
@@ -10,4 +10,5 @@ export interface IHandler extends Omit<ICommandUtilityParams, 'params'> { | |||
|
|||
export type IHanderParams = Omit<ICommandUtilityParams, 'params'> & { | |||
language: Language; | |||
params?: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why optinal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not answered ?
hey @VipinDevelops i've added the comments with detailed explanation of what changes i have made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requested more changes
don't tag me before you reply to all the old messages
README.md
Outdated
#### Creating Quick Replies from Message Box | ||
**`/quick create "<name>" <message>`**: Create a quick reply using the message box | ||
|
||
When you use this command: | ||
1. A modal window opens automatically | ||
2. The name and replybody field is prefilled with your specified values | ||
3. Simply review and click the Submit button to save your quick reply | ||
|
||
The command works as follows: | ||
- The name must be enclosed in quotes `"..."` if it contains multiple words | ||
- Everything after the name is treated as the message body | ||
- Without quotes, only the first word would be taken as the name | ||
|
||
#### Examples: | ||
```sh | ||
# Single word name (quotes optional) | ||
/quick create greeting Hello! How have you been? | ||
|
||
# Multi-word name (quotes required) | ||
/quick create "welcome message" Welcome to our channel! How can I help you? | ||
``` | ||
|
||
Both examples will open a modal with the name and message already filled in. Just click Submit to save your quick reply, which can then be used with **`/qs <name>`**. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is just to much for such simple thing
and can't we just do like
/quick create "you can use double quotes to write the name with spaces" you write everything else as a body ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this just looks like AI-generated markdown to me
@@ -10,4 +10,5 @@ export interface IHandler extends Omit<ICommandUtilityParams, 'params'> { | |||
|
|||
export type IHanderParams = Omit<ICommandUtilityParams, 'params'> & { | |||
language: Language; | |||
params?: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not answered ?
@@ -35,6 +35,7 @@ export class Handler implements IHandler { | |||
public triggerId?: string; | |||
public threadId?: string; | |||
public language: Language; | |||
public params?: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stilll ?
hey @VipinDevelops i have added examples as well |
I have also replied to the asked questions in the messages in the review section in each file still to ease it out why i added params as optional?
|
@VipinDevelops i was thinking about making a new pr for the same as this pr now has too many conversation messages and commits thus makes it a bit more clutterd and not easy to get through all the messages and conversation i am thinking if you allow me to make a new pr for this issue with the same commits and solution |
Super bad example Can't understand anything make it simple and short |
Just remove it if not needed why make it optional ? |
NO we won't make new PR's because of TOO MANY conversations |
How about this i have changed the examples and made them quite more clear |
Issue(s)
Closes #22
##This PR introduces a new feature that allows users to add the name and body of the new quick message from the rc rooms message box itself
Acceptance Criteria fulfillment
Proposed changes (including videos or screenshots)
/quick create "reply name" reply body
to have the reply value in the modal
Demo
Screencast.from.2025-02-23.02-37-52.webm
here i have shown the demo of all three cases
up for any changes and learning :)