-
Notifications
You must be signed in to change notification settings - Fork 96
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: add multiple options with one paste #1407
base: main
Are you sure you want to change the base?
Conversation
a958837
to
655301b
Compare
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.
Besides the specific layout problem, I also get some errors when I paste a blank line or remove the text from an option so that it's empty
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1407 +/- ##
============================================
- Coverage 45.02% 37.67% -7.36%
+ Complexity 720 514 -206
============================================
Files 61 49 -12
Lines 2776 1911 -865
============================================
- Hits 1250 720 -530
+ Misses 1526 1191 -335 |
I pushed a quick POC of @jotoeri 's idea.
If everyone agrees this is the way to go, I would appreciate some design advices. from @jancborchardt or anyone who has an input on the matter. Happy holidays everyone 🎄 |
@hamza221 I like the modal 🎉 Especially the representation of the options below the input field. I think it might be useful to show a text that makes it clear that you have to use new lines for new options. |
b378a00
to
fc511c4
Compare
I only rebased and fixed conflicts, still not ready for review |
fc511c4
to
2db438f
Compare
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 bubbles below with the recognized options (that's what it is, right?) could be using the regular tag style like in Mail. :)
Yes, that's what these are for :) good idea 👍🏻 |
@jancborchardt I think the NcSelect component fits our needs here as well and is less code to implement. I also added a loading indicator as it might take some time until all options get created on the server. |
ff1a3ff
to
edb6da2
Compare
src/components/OptionInputModal.vue
Outdated
padding: 20px; | ||
} | ||
|
||
:deep(.v-select) { |
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.
why this custom styles for the select?
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.
To have it the whole width of the dialog and a slightly bigger margin on top of it.
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.
@susnux Can we resolve this conversation or do you have a better solution for the styling?
571e7ef
to
1582bb4
Compare
1582bb4
to
231300c
Compare
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 very nice, just some details :)
- "Multiple options" should have a lowercase "o" as per "Sentence case"
- "Entered options" wording is a bit strange, maybe better "Options summary"
- The "Cancel" button should not have an icon as it’s not primary.
- The loading time on adding the options is quite long, can that be optimized somehow?
@jancborchardt We already looked into it. The problem is that we can only submit one option per API request at the moment. So we could do three things:
What about just "Options"? |
👍 |
I think this is hard as setting the order requires to know the total amount of options which is hard to validate on the backend, e.g. if option 5 arrives before option 4.
For the mentioned reason this is bad UX
I think this is the way to go |
@susnux ok, so we merge this and do a follow up for the new API version? |
88951b6
to
8d9a25c
Compare
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.
Sounds good to me then, thanks for the explanation. :)
f41fbb6
to
4231cf5
Compare
Signed-off-by: hamza221 <[email protected]> Signed-off-by: Christian Hartmann <[email protected]>
c85f8da
to
ae7eee0
Compare
This fixes #337 by adding a NcDialog that let's you paste multiple options into a textfield and adds a new option per line.
Unbenannt.webm
Signed-off-by: hamza221 [email protected]