-
Notifications
You must be signed in to change notification settings - Fork 156
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(mock-server): improve headers UI #1171
base: master
Are you sure you want to change the base?
feat(mock-server): improve headers UI #1171
Conversation
Signed-off-by: Yogesh01000100 <yogeshone678@gmail.com>
@rohanmathur91 @RuntimeTerror10 @wrongsahil can you help merging this pull request? |
@Yogesh01000100 Please attach Session Replay in the PR description |
@wrongsahil @RuntimeTerror10 please check the session, in the pull request description, (updated) |
@RuntimeTerror10 can you please check with the pull request that I put, and do let me know if any changes are required. |
@RuntimeTerror10 @wrongsahil can you check and help merge this PR? |
Hey @Yogesh01000100 , I will review it today and will share any changes if required by tomorrow. |
okay sure! |
Have you reviewed it? if any changes are there please inform me. |
<Col span={24}> | ||
<div style={{ marginBottom: "16px" }}> | ||
<Button onClick={addHeader} className="add-header"> | ||
<span style={{ marginRight: "8px", fontWeight: "bold" }}>+</span> |
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.
avoid using inline styles, use css class
const createHeadersString = (headersArray: Header[]): string => { | ||
const headersString: { [key: string]: Header } = {}; | ||
|
||
headersArray.forEach((header, index) => { | ||
headersString[index.toString()] = { | ||
name: header.name, | ||
value: header.value, | ||
index: index + 1, | ||
}; | ||
}); | ||
|
||
return JSON.stringify(headersString); | ||
}; | ||
|
||
const addHeader = () => { | ||
const newHeader: Header = { | ||
name: "", | ||
value: "", | ||
index: mappedHeader.length, | ||
}; | ||
const updatedHeaders: Header[] = [...mappedHeader, newHeader]; | ||
setMappedHeader(updatedHeaders); | ||
setHeadersString(createHeadersString(updatedHeaders)); | ||
}; | ||
|
||
const removeHeader = (indexNum: number) => { | ||
const updatedHeaders = [...mappedHeader]; | ||
updatedHeaders.splice(indexNum, 1); | ||
|
||
const updatedHeadersString = createHeadersString(updatedHeaders); | ||
setHeadersString(updatedHeadersString); | ||
if (errors && errors.headers && errors.headers.length > 0) { | ||
const newError = errors.headers.filter((item) => { | ||
if (item.indexError === indexNum) { | ||
return false; | ||
} else if (item.indexError > indexNum) { | ||
item.indexError--; | ||
} | ||
return true; | ||
}); | ||
setErrors({ ...errors, headers: newError }); | ||
} | ||
}; | ||
|
||
const updateHeaders = (value: string, index: number, fieldToUpdate: string) => { | ||
const updatedHeaders = [...mappedHeader]; | ||
if (fieldToUpdate === "name") { | ||
updatedHeaders[index].name = value; | ||
} else if (fieldToUpdate === "value") { | ||
updatedHeaders[index].value = value; | ||
} | ||
setMappedHeader(updatedHeaders); | ||
setHeadersString(JSON.stringify(updatedHeaders)); | ||
}; |
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.
All these create
/add
/update
header functions should defined and handled inside HeaderSection
instead of the root component, the root component should get the new changes in headers when any change is made. Pass the state setter to this component and on headers change call the state setter function with the latest header changes
updateHeaders={updateHeaders} | ||
removeHeader={removeHeader} | ||
addHeader={addHeader} |
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 need to pass these functions as props, just pass the state setter functions setMappedHeader
setHeadersString
and call them with the latest header changes when any operation is performed on headers.
const removeHeader = (indexNum: number) => { | ||
const updatedHeaders = [...mappedHeader]; | ||
updatedHeaders.splice(indexNum, 1); | ||
|
||
const updatedHeadersString = createHeadersString(updatedHeaders); | ||
setHeadersString(updatedHeadersString); | ||
if (errors && errors.headers && errors.headers.length > 0) { | ||
const newError = errors.headers.filter((item) => { | ||
if (item.indexError === indexNum) { | ||
return false; | ||
} else if (item.indexError > indexNum) { | ||
item.indexError--; | ||
} | ||
return true; | ||
}); | ||
setErrors({ ...errors, headers: newError }); | ||
} | ||
}; |
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.
Wrap this with useCallback
const updateHeaders = (value: string, index: number, fieldToUpdate: string) => { | ||
const updatedHeaders = [...mappedHeader]; | ||
if (fieldToUpdate === "name") { | ||
updatedHeaders[index].name = value; | ||
} else if (fieldToUpdate === "value") { | ||
updatedHeaders[index].value = value; | ||
} | ||
setMappedHeader(updatedHeaders); | ||
setHeadersString(JSON.stringify(updatedHeaders)); | ||
}; |
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.
Wrap this with useCallback
export const validateHeaders = (headers: { [key: string]: string }) => { | ||
const headerArray = Object.values(headers).map((item) => { | ||
if (item && typeof item === "object") { | ||
//@ts-ignore |
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.
Try to avoid using this flag
const HeaderErrors = []; | ||
const seenHeaderNames = new Set(); | ||
|
||
for (let i = 0; i < headerArray.length; i++) { |
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.
Avoid using for loops, use ES6 methods like map
, filter,
forEach
<div key={index}> | ||
{errors.headers | ||
.filter((err) => err.indexError === index) | ||
.map((headerError, errorIndex) => { | ||
if (headerError.valueError.startsWith(errorType)) { | ||
const boxPosError = headerError.valueError.slice(errorType.length).trim(); | ||
return <div key={errorIndex}>{boxPosError}</div>; | ||
} | ||
return null; | ||
})} | ||
</div> | ||
); | ||
} | ||
return null; | ||
}; |
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 should not perform checks on strings to determine if the error is for header name
or value
, error messages might change in future which will break this.
const error: { typeOfError?: string; errorIndex?: number } = {}; | ||
|
||
if (!header.name) { | ||
error.typeOfError = "name Header name is required"; |
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.
Set type of error to either name
or value
, and add other property to error object named description
which will be rendered below respective inputs.
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.
Also remove the error type suffix added on each error message
@Yogesh01000100 Suggested UX change: |
@Yogesh01000100 Added comments and one UX change |
- Implemented automatic addition of new header fields upon user input in the last header. - Refactored functions from parent to child components for better code organization. - Updated syntax to ES6 for consistency and readability. - Utilized useCallback for optimized rendering and performance. Signed-off-by: D.Yogesh <yogeshone678@gmail.com>
I've implemented the requested changes and added the new automatic header field creation upon user input. |
Hi @RuntimeTerror10 I'm reaching out to inquire about the status of the review for this pull request. If there are specific concerns or changes needed, I'm more than willing to address them. |
Hey @Yogesh01000100 |
Hi @RuntimeTerror10 I understand that you're likely juggling a lot of responsibilities, and I appreciate the effort it takes to manage this project. If there have been any developments regarding the review, or if there's any additional information or modifications you need from my end, please let me know. I'm eager to contribute and to make any necessary adjustments to my submission. |
Closes issue: #1134
📜 Summary of changes:
This pull request addresses the following changes related to header creation:
Screen Replay session
https://app.requestly.io/sessions/saved/4diq8jHpNFIkFLebGCSE
screen-replay-session-headers-ui.mp4
✅ Checklist:
🧪 Test instructions:
To test these changes, follow these steps: