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: emit height difference of NcReferenceList after loading. #5730

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DorraJaouad
Copy link
Contributor

☑️ Resolves

Needed for nextcloud/spreed#10627. Link previews are adding extra height which causes jumping in the chat scroll.

🖼️ Screenshots

No visual changes

🚧 Tasks

  • event loaded now has height difference as a payload [CHANGED BEHAVIOR]

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
@DorraJaouad DorraJaouad added enhancement New feature or request 3. to review Waiting for reviews feature: richtext Related to the richtext component labels Jun 21, 2024
@DorraJaouad DorraJaouad added this to the 8.13.0 milestone Jun 21, 2024
@DorraJaouad DorraJaouad self-assigned this Jun 21, 2024
this.loading = false
this.$emit('loaded')
this.$emit('loaded', height)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say reporting the height is not really a expected payload of the event, no?
Because you do not really need it (see below).

Comment on lines +390 to +396
handleReferenceListLoaded(oldHeight) {
this.$nextTick(() => {
// emit height difference to parent component
const heightDiff = this.$refs.referenceList.$el.offsetHeight - oldHeight
this.$emit('reference-list-loaded', heightDiff)
})
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
handleReferenceListLoaded(oldHeight) {
this.$nextTick(() => {
// emit height difference to parent component
const heightDiff = this.$refs.referenceList.$el.offsetHeight - oldHeight
this.$emit('reference-list-loaded', heightDiff)
})
},
handleReferenceListLoaded() {
const oldHeight = this.$refs.referenceList.$el.offsetHeight
this.$nextTick(() => {
// emit height difference to parent component
const heightDiff = this.$refs.referenceList.$el.offsetHeight - oldHeight
this.$emit('reference-list-loaded', heightDiff)
})
},

Copy link
Contributor

Choose a reason for hiding this comment

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

But in general would it not just work if you emit any event here and in Talk react on that element by scrolling the element into view?
Then you do not need any height calculation here

Copy link
Contributor Author

@DorraJaouad DorraJaouad Jun 21, 2024

Choose a reason for hiding this comment

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

calculating the old height here may be already updated with the reference data added height ?

in Talk react on that element by scrolling the element into view?

The goal not to scroll to the element but to calculate the extra height added (it will scroll the view above its previous position) and then we scroll back to the right position.

Copy link
Contributor

Choose a reason for hiding this comment

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

calculating the old height here may be already updated with the reference data added height ?

I am not sure about the timing of Vue's event handling, sadly I could not find good documentation about this. Maybe @ShGKme knows more?

The goal not to scroll to the element but to calculate the extra height added (it will scroll the view above its previous position) and then we scroll back to the right position.

I do not know the richtext part good enough (and the talk code) so your solution might be the best.
But then I still would not pass the height as magic number to the event but use a context object with proper property naming like

this.$emit('reference-list-loaded', { heightDiff })

@susnux susnux modified the milestones: 8.13.0, 8.14.0 Jun 25, 2024
@susnux susnux modified the milestones: 8.14.0, 8.15.0 Jul 4, 2024
@DorraJaouad DorraJaouad marked this pull request as draft July 4, 2024 21:09
@Antreesy Antreesy modified the milestones: 8.15.0, 8.15.1 Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: richtext Related to the richtext component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants