-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[FEATURE] Improvement - char limit on tags #9762
Conversation
@pratik9818 I have noticed there are so many formatting issues in the files where you have made changes. Please format your code and push the formatted 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.
Looks good
hi @SaraJaoude , I completely understand you're busy. I would be grateful if you could take some time to review the pull request because i love to work on another issue so it would be great if this will get merge.I look forward to hearing your thoughts on the changes if something needs to add or remove in code . |
cls
Outdated
branch.main.remote=origin | ||
branch.main.merge=refs/heads/main | ||
branch.char.remote=origin | ||
branch.char.merge=refs/heads/char |
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 don't think this file is needed - please remove and add it to your gitignore
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.
ok
components/tag/TagsInput.js
Outdated
@@ -11,10 +18,22 @@ export default function TagsInput({ tags, onTagAdd, onTagRemove }) { | |||
backspace: 8, | |||
}; | |||
|
|||
const maxTagChar = 32; |
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 think this can be renamed to maxLength
because it is already in the tag component
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.
ok
components/tag/TagsInput.js
Outdated
if (e.keyCode === comma || inputValue.endsWith(",")) { | ||
const newTag = inputValue.trim().replace(/,/g, ""); | ||
let newTag = inputValue.trim().replace(/,/g, ""); |
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 is this changed 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.
@eddiejaoude , should i update the tag description in event page as well , like a profile page |
yes please make it consistent Can you also resolve the conflicts please |
Did you mean to close the PR? No need to force push, commits will be squashed, plus this makes reviewing very difficult |
i did not do nothing, i don't know how it happend , what should i do now ? |
@eddiejaoude , i reopend the PR |
Oh your force push undid my changes I made 😞 - I will look to make them again soon |
Apology @eddiejaoude , i will keep in mind next time . But i made the all changes you said to changed |
no problem, I made additional changes in addition to the comments |
hi @eddiejaoude what should i do now , i mean, is their any changes i have to do ? |
pages/account/manage/profile.js
Outdated
@@ -247,8 +247,7 @@ export default function Profile({ BASE_URL, profile, fileExists }) { | |||
setShowNotification={setShowNotification} | |||
/> | |||
<p className="text-sm text-primary-medium-low dark:text-primary-low-high"> | |||
Separate tags with commas and Maximum 32 characters | |||
allowed in each tag. | |||
Separate tags with commas (max 32 characters). |
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 do my inline changes keep getting removed?
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.
The additional changes seem to have moved the PR backwards and the package lock
file has also been added, please remove
i am working on those issues |
Ok if you need any help let us know |
@eddiejaoude can i restart my char branch like updating char branch same as this project main branch because i think i am unable to update or back all those changes which u mentioned, or create new PR. can u help me out from this. |
Status - still fixing it. |
@eddiejaoude i fixed the issue and i had to hard push code for solveing that issue . kindly can you check ? |
Looks good, thank you for fixing, but force push is probably what is causing all the problems. I left a tiny inline comment |
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.
Thank you 👍
Thank you @eddiejaoude for supporting |
Fixes Issue
closes #9345
Changes proposed
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers