Uh oh!
There was an error while loading. Please reload this page.
feat: add labels to textarea elements in translate modal - #5234
Conversation
There was a problem hiding this comment.
The autosize method seems to be broken due to that, you can check that by trying to type something in the input.
We probably can drop it and instead use resize from the component https://nextcloud-vue-components.netlify.app/#/Components/NcFields?id=nctextarea just need to ensure that the max height is limited to not exceed the visible screen space in the modal
5c1b783 to
67c2d3cCompare67c2d3c to
a776275Comparea776275 to
ba0adf3Compareemoral435
commented
Jan 16, 2024
Since this issue deals with accessibility, would you want the rest of the accessibility team to also help review? :) |
juliusknorr
commented
Jan 22, 2024
@emoral435 Definitely, additional reviews are very welcome |
emoral435
left a comment
There was a problem hiding this comment.
Other than these general questions, LGTM! Will ping other, more experiences a11y team members too :)
| </script> | ||
| <style lang="scss" scoped> | ||
| <style lang="scss"> |
There was a problem hiding this comment.
question: Any reason you removed this CSS being scoped? Just for best practices, I know this is only a text modal, but keeping it scoped will help ensure nothing funky happens in case there are similar classes
There was a problem hiding this comment.
especially if you are adding the !important elements in the CSS, this indicates that other pieces of css that may not be scoped are affecting this code block, then having this be scoped helps prevent similar things in the future
There was a problem hiding this comment.
Agree with @emoral435, scoped should not be removed. These styles are not about something really global.
You can use :deep: https://vuejs.org/api/sfc-css-features.html#deep-selectors
Best to specify a selector on this component and then with deep selector on manually provided class.
.translate-dialog:deep(.translate-textarea)
/* or more specific, set class on the NcTextarea and then select on textarea */
.translate-dialog__textarea:deep(.translate-textarea)ba0adf3 to
7eebe32Compare| <NcTextArea ref="input" | ||
| v-model="inputValue" | ||
| :label="t('text', 'Text to translate from')" | ||
| autofocus | ||
| @input="autosize" | ||
| input-class="translate-textarea" | ||
| resize="none" |
There was a problem hiding this comment.
It works, but it is incorrect usage of the component. NcTextArea, same as other fields in @nextcloud/vue, doesn't have v-model - it has no defined model and doesn't emit input event.
It works, because here v-model directive catches native event input that bubbles with the native event. What it why it was needed to add computed proxy.
Instead, .sync modifier should be used on value binding: :value.sync="input".
| inputValue: { | ||
| get() { | ||
| return this.input | ||
| }, | ||
| set(event) { | ||
| this.input = event.target.value | ||
| this.autosize() | ||
| }, | ||
| }, |
There was a problem hiding this comment.
If we want to resize on input, we can watch the input property, so it won't be important how the value has changed for the resize.
ShGKme
commented
Jan 24, 2024
Also, I cannot focus any input in the modal, but I cannot say if it isn't an issue on my side... |
luka-nextcloud
commented
Jan 25, 2024
Sorry, I cannot reproduce the issue on my side. |
Signed-off-by: Luka Trovic <luka@nextcloud.com>
7eebe32 to
d29c967Compareemoral435
commented
Jan 26, 2024
With two reviews, should be good to merge now @luka-nextcloud 👍 |
JuliaKirschenheuter
commented
Jan 26, 2024
/backport to stable28 |
emoral435
commented
Jan 26, 2024
/backport d29c967 to stable28 |
juliusknorr
commented
Jan 29, 2024
/backport to stable28 |
📝 Summary
🖼️ Screenshots
🚧 TODO
🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)