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

feat(chatbar): **markdown** in chatbar #1040

Merged
merged 16 commits into from
Jan 31, 2022
Merged

feat(chatbar): **markdown** in chatbar #1040

merged 16 commits into from
Jan 31, 2022

Conversation

molimauro
Copy link
Contributor

@molimauro molimauro commented Jan 19, 2022

What this PR does 📖
Chatbar markdown, chatbar rework, css bugfix, integrating old features, emoji fix. Focus input on reply, recipient switch, emoji selection.

Which issue(s) this PR fixes 🔨
AP-2
AP-196
AP-474

Special notes for reviewers 🗒️
Available markdown:
italics with * and _
bold **
underline __
escape command
code with single `
combined bold/italics ***
strikethrough ~~
autolinks <https://satellite.im>
url parsing
spoilers ||

Additional comments 🎤
Related to the chatbar, I’m going to open the following tickets:

  • keep track of last cursor position for different purposes (example: emoji are always inserted at the end of the string);
  • remove marked (when you send a message it will be converted in the chat to html by marked, must be refactored and removed);
  • undo/redo manager (cmd + z, shift + cmd + z);
  • integrate highlightjs;
  • integrate mentions ui (@user1, @here, @channel..).

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Jan 19, 2022
@phillsatellite
Copy link
Contributor

phillsatellite commented Jan 19, 2022

emoji.mov
spoilers.mov

Only 2 issue I noticed going through was Emoji is showing up tiny in the chatbar and when you select an emoji the menu will move to the opposite side of the screen and spoilers was not working correctly for me either (verified this problem only happens on my local and not dev) otherwise all good

@stavares843 stavares843 added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Jan 19, 2022
@molimauro
Copy link
Contributor Author

molimauro commented Jan 20, 2022

emoji.mov
spoilers.mov
Only 2 issue I noticed going through was Emoji is showing up tiny in the chatbar and when you select an emoji the menu will move to the opposite side of the screen and spoilers was not working correctly for me either (verified this problem only happens on my local and not dev) otherwise all good

For the emoji i'll check. What do you mean by "tiny"? How should they appear?

I see the spoilers are working correctly in the chatbar, they do not work correctly once you send the message because that part is still a WIP.

@phillsatellite
Copy link
Contributor

emoji.mov
spoilers.mov
Only 2 issue I noticed going through was Emoji is showing up tiny in the chatbar and when you select an emoji the menu will move to the opposite side of the screen and spoilers was not working correctly for me either (verified this problem only happens on my local and not dev) otherwise all good

For the emoji i'll check. What do you mean by "tiny"? How should they appear?

I see the spoilers are working correctly in the chatbar, they do not work correctly once you send the message because that part is still a WIP.

I think the emojis looking too small was just me so you can just scratch that lol and noted about the spoilers 👍

@stavares843
Copy link
Member

thanks for testing @phillsatellite 🔨

@molimauro thanks for clarifying, is there anything else that is a WIP?

@molimauro
Copy link
Contributor Author

emoji.mov
spoilers.mov
Only 2 issue I noticed going through was Emoji is showing up tiny in the chatbar and when you select an emoji the menu will move to the opposite side of the screen and spoilers was not working correctly for me either (verified this problem only happens on my local and not dev) otherwise all good

For the emoji i'll check. What do you mean by "tiny"? How should they appear?
I see the spoilers are working correctly in the chatbar, they do not work correctly once you send the message because that part is still a WIP.

I think the emojis looking too small was just me so you can just scratch that lol and noted about the spoilers 👍

Great!! Thanks :)

@molimauro
Copy link
Contributor Author

molimauro commented Jan 20, 2022

@stavares843

This PR was meant to refactor the chatbar, the sent messages must be parsed according the new markdown rules created.
You can find whats missing in "Additional comments"! I've already created the corresponding tickets, if there's something not clear let me know! thanks! ;)

@stavares843
Copy link
Member

@molimauro yes, both Phil and I checked the additional comments, but there was nothing there regarding the spoilers being a WIP, thats why im asking to double check 😂

@molimauro
Copy link
Contributor Author

@stavares843 yeah sorry, i should have been more specific. Spoilers and underlines are "special" md rules i have added. They are parsed correctly only in the chatbar and not when sent :)

@stavares843 stavares843 removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Jan 20, 2022
@molimauro
Copy link
Contributor Author

@phillsatellite i should've fixed the enhancer problem :)

test/Markdown.test.ts Outdated Show resolved Hide resolved
@phillsatellite phillsatellite added the QA tested One QA Person has tested - Needs QA Lead review still label Jan 24, 2022
@stavares843 stavares843 added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Jan 24, 2022
store/ui/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@WanderingHogan WanderingHogan left a comment

Choose a reason for hiding this comment

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

It looks great, there are a couple minor things you could change if you wanted, but up to you

@WanderingHogan WanderingHogan removed the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Jan 25, 2022
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Jan 25, 2022
@stavares843
Copy link
Member

Captura de ecrã 2022-01-25, às 17 53 21

Captura de ecrã 2022-01-25, às 17 53 32

first is this branch, second is dev branch

the chat bar has a bigger height compared with dev, is that modification in purpose?

@molimauro
Copy link
Contributor Author

Captura de ecrã 2022-01-25, às 17 53 21 Captura de ecrã 2022-01-25, às 17 53 32

first is this branch, second is dev branch

the chat bar has a bigger height compared with dev, is that modification in purpose?

yes i noticed on the figma that the chatbar was slightly bigger and that the placeholder was gray. I can change it of course :)

@stavares843
Copy link
Member

neat, thanks for checking, just confirming 🎨

@stavares843
Copy link
Member

I checked with the design team and there was an error within Figma

it should be 48px high. with a 16px padding all around the inside

Screen_Shot_2022-01-25_at_12 32 20_PM

@molimauro
Copy link
Contributor Author

great job @stavares843, thanks 🎉

@stavares843 stavares843 removed the QA tested One QA Person has tested - Needs QA Lead review still label Jan 27, 2022
@stavares843
Copy link
Member

are you gonna address that layout change? 🔨 let me know if you need more details

also, conflict 🔨

@molimauro
Copy link
Contributor Author

are you gonna address that layout change? 🔨 let me know if you need more details

also, conflict 🔨

yes, i'm doing it! I asked to Liz because i had to reduce the padding but kept the 48px height. I'll resolve also other minor fixes i discovered (like adding a max-height on the chat, otherwise if you keep adding a lot of paragraphs it's not very good) and then i rebase and push. Thanks!!

@iltumio
Copy link
Collaborator

iltumio commented Jan 30, 2022

Seems that we have an overlap with the right icons

Schermata 2022-01-30 alle 20 35 27

@molimauro
Copy link
Contributor Author

Captura de ecrã 2022-01-28, às 23 13 23

also, the spoilers are not working here, is that part still a WIP?

In this branch spoilers and underlines are managed only in the chatbar, they're not parsed when the msg is sent

@molimauro
Copy link
Contributor Author

Done, thanks guys!!

@iltumio iltumio added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Jan 31, 2022
@phillsatellite phillsatellite added QA tested One QA Person has tested - Needs QA Lead review still and removed Ready for QA Ready for QA team to test, Devs approved. labels Jan 31, 2022
@stavares843 stavares843 merged commit 953620b into dev Jan 31, 2022
@stavares843 stavares843 deleted the feature_md_chatbar branch January 31, 2022 16:48
@github-actions github-actions bot removed the QA tested One QA Person has tested - Needs QA Lead review still label Jan 31, 2022
pavlzk pushed a commit that referenced this pull request Feb 4, 2022
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.

None yet

6 participants