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

Gn 4901 autofill variable #472

Merged
merged 32 commits into from
Oct 4, 2024
Merged

Gn 4901 autofill variable #472

merged 32 commits into from
Oct 4, 2024

Conversation

lagartoverde
Copy link
Contributor

@lagartoverde lagartoverde commented Sep 3, 2024

Overview

Made a new variable that get's autofilled if the correct context is provided

connected issues and PRs:

GN-4901

Setup

How to test/reproduce

Challenges/uncertainties

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check if dummy app is correctly updated
  • Check cancel/go-back flows
  • changelog
  • npm lint
  • no new deprecations

@lagartoverde lagartoverde self-assigned this Sep 3, 2024
@lagartoverde lagartoverde added the enhancement New feature or request label Sep 3, 2024
@lagartoverde
Copy link
Contributor Author

Copy link
Collaborator

@piemonkey piemonkey left a comment

Choose a reason for hiding this comment

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

Works well but some comments on the UX.

Also, I think we need an edit component for this to be useful, as currently there's no easy way to modify it, you need to insert a new one.

addon/components/variable-plugin/autofilled/insert.hbs Outdated Show resolved Hide resolved
<AuLabel for='autofill_key'>
{{t 'variable-plugin.autofill.autofillKey'}}
</AuLabel>
<AuNativeInput
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would make more sense to me as a powerselect, with the possible values taken from configuration. otherwise there is too much margin for error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed with Elena but will be happy to discuss with you in a standup or we can set up a meeting, the idea is that the plugin is as flexible as possible and the template app have no idea which final values will be implemented in the filler app, this of course has inconvenients as it has margin for error

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that makes sense but it still seems error-prone since we know at least some common ones that are likely to exist. Happy to just go with a text input though.

class='variable'
{{on 'click' this.onClick}}
>
<EmberNode::EmbeddedEditor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we actually need an embedded editor? As it is you can actually replace the content with text, which does correctly get replaced with the value, but it still seems confusing to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed that, thank you for spotting the problem :)

Copy link
Collaborator

@piemonkey piemonkey Sep 30, 2024

Choose a reason for hiding this comment

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

It seems like you understood something different than I meant from my comment, but I'm glad it helped spot a bug :)

What I meant was, that I don't see why we want an embedded editor here at all. To my mind, the contents of the variable should only be changeable by setting a different key or label. Here's a screenshot to demonstrate what I mean. This is a document with 2 autofill variables. You can see that I typed over the contents of the second, which means the only way to know that it's an autofill variable is to look in the sidebar. Is there some reason that template builders would want to be able to do this that I am missing?

image

@lagartoverde
Copy link
Contributor Author

Ok, so after rewriting a lot of the code, the variable will be initialized only once, so the user can modify it and it won't override its changes

Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

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

some translation improvements

translations/nl-BE.yaml Outdated Show resolved Hide resolved
translations/nl-BE.yaml Outdated Show resolved Hide resolved
translations/nl-BE.yaml Outdated Show resolved Hide resolved
translations/en-US.yaml Outdated Show resolved Hide resolved
@abeforgit
Copy link
Member

@lagartoverde you can merge after you do the translations

@lagartoverde lagartoverde enabled auto-merge October 4, 2024 15:05
@lagartoverde lagartoverde merged commit 7d91835 into master Oct 4, 2024
3 checks passed
@lagartoverde lagartoverde deleted the GN-4901-autofill-variable branch October 4, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants