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
luka-nextcloud
force-pushed
the
feature/add-labels-to-translate-textarea
branch
from
5c1b783
to
67c2d3c
Compare
luka-nextcloud
force-pushed
the
feature/add-labels-to-translate-textarea
branch
from
67c2d3c
to
a776275
Compare
juliusknorr
force-pushed
the
feature/add-labels-to-translate-textarea
branch
from
a776275
to
ba0adf3
Compare
|
Since this issue deals with accessibility, would you want the rest of the accessibility team to also help review? :) |
|
@emoral435 Definitely, additional reviews are very welcome |
src/components/Modal/Translate.vue
Outdated
| @@ -210,7 +225,7 @@ export default { | |||
| } | |||
| </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)
luka-nextcloud
force-pushed
the
feature/add-labels-to-translate-textarea
branch
from
ba0adf3
to
7eebe32
Compare
src/components/Modal/Translate.vue
Outdated
| <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".
src/components/Modal/Translate.vue
Outdated
| 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.
|
Also, I cannot focus any input in the modal, but I cannot say if it isn't an issue on my side... |
Sorry, I cannot reproduce the issue on my side. |
Signed-off-by: Luka Trovic <[email protected]>
luka-nextcloud
force-pushed
the
feature/add-labels-to-translate-textarea
branch
from
7eebe32
to
d29c967
Compare
|
With two reviews, should be good to merge now @luka-nextcloud 👍 |
|
/backport to stable28 |
|
/backport d29c967 to stable28 |
|
/backport to stable28 |
📝 Summary
🖼️ Screenshots
🚧 TODO
🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)