Skip to content

Refactor NewMessageForm component #9430

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.

Sign up for GitHub

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

Jump to bottom
Merged
Antreesy merged 7 commits into from
May 12, 2023
Merged

Refactor NewMessageForm component #9430

Antreesy merged 7 commits into from
May 12, 2023

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented May 2, 2023 •
edited
Loading

☑️ Resolves

🚧 Tasks

  • Splitted by commits for easier review 💙
  • Rename existing components according to Vue styleguide
  • Minor renamings of properties, typo and lint fixes
  • Extract attachments NcActions to separate component
  • Extract text creation NcModal to separate component
  • Visual check
  • Code review

🏁 Checklist

@Antreesy Antreesy self-assigned this May 2, 2023
@Antreesy Antreesy requested review from ShGKme and marcoambrosini May 2, 2023 17:27
@Antreesy Antreesy added this to the 💙 Next Major (27) milestone May 2, 2023
@Antreesy Antreesy marked this pull request as ready for review May 3, 2023 09:05
@Antreesy Antreesy mentioned this pull request May 5, 2023
Merged
15 tasks
@Antreesy Antreesy force-pushed the fix/7493/refactor-new-message-form branch from a27f6da to 55d7ef3 Compare May 5, 2023 21:38
@Antreesy
Copy link
Contributor Author

Antreesy commented May 5, 2023 •
edited
Loading

  • Rebased onto master
  • Move recently added composable for viewer to child component

To discuss:

Shall we rename a parent component to make all names shorter?

─ NewMessage
   ├── NewMessageAttachments.vue
   ├── NewMessageAudioRecorder.vue
   ├── NewMessagePollEditor.vue
   ├── NewMessageTemplatePreview.vue
   ├── NewMessageTextCreateDialog.vue
   ├── NewMessageTypingIndicator.vue
   ├── NewMessageUploadEditor.vue
   └── NewMessage.vue

It requires changes only in three additional files:

 M BreakoutRoomsEditor/SendMessageDialog.vue
 M ChatView.vue
 M Quote.vue

@Antreesy Antreesy mentioned this pull request May 10, 2023
Closed
@Antreesy Antreesy force-pushed the fix/7493/refactor-new-message-form branch from 55d7ef3 to de09c9c Compare May 11, 2023 09:05
@Antreesy
Copy link
Contributor Author

Antreesy commented May 11, 2023 •
edited
Loading

  • Rebased onto master (with TypingIndicator)
  • Add commits to resolve minor issues (see description)

nickvergessen
nickvergessen reviewed May 11, 2023
View reviewed changes
ShGKme
ShGKme reviewed May 12, 2023
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, makes this super component simpler and smaller.
I added some not blocking comments.

Tested, that all still work in main chat view and breakout rooms.

src/components/NewMessageForm/NewMessageFormAttachments.vue Outdated
Comment on lines 35 to 37
<NcActionButton v-if="canUploadFiles"
close-after-click
@click.prevent="$emit('open-file-upload')">
Copy link
Contributor

@ShGKme ShGKme May 11, 2023

Choose a reason for hiding this comment

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

Prevent modifier cannot be used on Vue component events, only on native DOM events.

Copy link
Contributor Author

@Antreesy Antreesy May 12, 2023

Choose a reason for hiding this comment

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

We have a lot of NcButtons with @click.prevent, should be a follow-up

@ShGKme
Copy link
Contributor

ShGKme commented May 12, 2023 •
edited
Loading

Shall we rename a parent component to make all names shorter?

I agree to rename it. It's shorter, the component is actually more, than a form (includes attachments, and a voice message), and its interface is not similar to the form interface (no submit event or reset method etc).

@marcoambrosini what do you think?

@Antreesy Antreesy requested a review from ShGKme May 12, 2023 14:56
@Antreesy Antreesy enabled auto-merge May 12, 2023 15:11
@Antreesy Antreesy force-pushed the fix/7493/refactor-new-message-form branch from fbcd9a9 to 1ffd8e5 Compare May 12, 2023 16:27
@Antreesy
Copy link
Contributor Author

Antreesy commented May 12, 2023

Rebased onto master because of conflict with #9526
Commits after review are here: 5ae4c82, 1ffd8e5

@ShGKme ShGKme disabled auto-merge May 12, 2023 16:53
@ShGKme
Copy link
Contributor

ShGKme commented May 12, 2023

I disabled auto-merge, as it is a bit large refactoring in case @marcoambrosini would want to review it too.

Feel free to re-enable if you want it to be in the beta release.

ShGKme
ShGKme approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Still works after changes and rebase 😀

src/components/NewMessageForm/NewMessageFormPollEditor.vue
const indexOfNewPollOption = this.pollOptions.length - 1
const refOfNewPollOption = `pollOption${indexOfNewPollOption}`
this.$refs[refOfNewPollOption][0].$el.querySelector('.input-field__input').focus()
this.$refs.pollOption[this.pollOptions.length - 1].$el.querySelector('.input-field__input').focus()
Copy link
Contributor

@ShGKme ShGKme May 12, 2023

Choose a reason for hiding this comment

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

In the current ES the last element is easy to get with at

Suggested change
this.$refs.pollOption[this.pollOptions.length - 1].$el.querySelector('.input-field__input').focus()
this.$refs.pollOption.at(-1).$el.querySelector('.input-field__input').focus()

@Antreesy Antreesy merged commit 7166cca into master May 12, 2023
@Antreesy Antreesy deleted the fix/7493/refactor-new-message-form branch May 12, 2023 16:56
@Antreesy Antreesy mentioned this pull request May 12, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@ShGKme ShGKme ShGKme approved these changes

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

Assignees

@Antreesy Antreesy

Labels

3. to review enhancement feature: frontend 🖌️ "Web UI" client

Projects

None yet

Milestone

v18.0.0-beta.1

Development

Successfully merging this pull request may close these issues.

Adjust layout of the template picker with server update Special symbols replaced when use EmojiPicker Simplify new message form

4 participants

@Antreesy @ShGKme @nickvergessen