-
Notifications
You must be signed in to change notification settings - Fork 26
Fixes layouts #93
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
Fixes layouts #93
Conversation
…ation and ability to override default values like color
✅ Deploy Preview for jsonforms-vuetify-renderers ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@sdirix @lucas-koehler please review |
@sdirix will you have some time next week to review this and if good to have a release ? Also did you decide if we want to include the FileRenderer and perhaps close the PR for the TemplateRenderer if indeed you do not want that added to this project like the webcomponent. |
Hello @kchobantonov , sorry for the late reply and thanks again for the contributions. We are going to review this PR and create a release in the next two weeks. We decided to not introduce the template renderer. I'll close that PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general! I would like to suggest to refactor this PR next week as suggested when the next prerelease of JSON Forms is published.
@@ -110,7 +114,7 @@ | |||
<v-icon class="notranslate">mdi-arrow-up</v-icon> | |||
</v-btn> | |||
</template> | |||
Move Up | |||
{{ t('array.btn.moveUp.tooltip', 'Move Up') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just merged a PR on the main repository which provides these translations: eclipsesource/jsonforms#2129
We'll release on the main repository next week, then we can update this PR to consume these translations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdirix I have changed most of the translations but for the up/down tooltip we are missing the properties - we have only the up/down for the aria label - or should I use that for now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. let's keep it like this for now
@@ -42,18 +42,28 @@ | |||
|
|||
<v-dialog v-model="dialog" persistent max-width="600" @keydown.esc="cancel"> | |||
<v-card> | |||
<v-card-title class="text-h5"> Clear form? </v-card-title> | |||
<v-card-title class="text-h5"> | |||
{{ t('form.clear.title', 'Clear form?') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should ideally then work in a similar way like the mentioned PR above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find the properties for that in the jsonforms core yet or maybe I'm not looking at the right place - can you point me to the locations in the jsonforms core where this is defined the same way how we have the arrayTranslations ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea would be to handle this via the binding and pass down the translated labels instead of calling the translator within the renderer. But for now I'm fine with this.
@sdirix I have updated this branch to use jsonforms 3.1.0-alpha.3 and I added the suggested changes for the translations except for the tooltips for array.btn.moveUp.tooltip I couldn't find any any translation properties for the up/down tooltip - only for the aria label |
@lucas-koehler @sdirix can you please review - I have included the changes from #92 as well as the suggestions for the translation except for few of those that I couldn't find. Also any plans for a release for this project where this PR is included ? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on first glance and couldn't see any regression. We can probably merge and release on Friday or Monday 👍
Released as part of |
Fixing layouts, translations, minor fixes