-
Notifications
You must be signed in to change notification settings - Fork 103
[KTextbox] Add slots to append elements before or after input #1123
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
Conversation
|
Thank you @Abhishek-Punhani. I've just returned from a vacation and need some time to catch up on everything. Then I will review. |
Sure. Let me know if any changes are required |
|
@Abhishek-Punhani would you be able to add visual tests and documentation examples as mentioned in the issue's requirements? It would help me to test everything. |
Percy Visual Test ResultsPercy Dashboard: View Detailed Report Environment:
Instructions for Reviewers:
|
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.
Thank you @Abhishek-Punhani, overall this direction looks good to me. I left few initial notes. I appreciated the screenshot, even though to be able to merge this feature, we would need to have visual tests and documentation updated anyway. I will do one more detailed review round as soon as those are in place - it will help me to confirm all works as expected.
@MisRob, I will also add visual tests for this feature. Could you please provide some guidance on updating documentation? I just need to add docsExample demonstrating all slots, right? |
|
Thank you @Abhishek-Punhani.
Exactly. And actually - example files are shared between visual tests and documentation :) It's explained in the visual testing guidance that we've recently updated - so I recommend to follow step by step. Then you can just import those very same examples to the documentation. No need to write much - I will fine-tune documentation details before we merge if needed. |
e119136 to
a3f869f
Compare
|
@MisRob, I have added FAIL lib/KTextbox/__tests__/visual.spec.js
● Test suite failed to run
The error below may be caused by using the wrong test environment, see https://jestjs.io/docs/configuration#testenvironment-string.
Consider using the "jsdom" test environment.
ReferenceError: document is not defined
68 | };
69 |
> 70 | if (document.readyState === 'loading') {
| ^
71 | document.addEventListener('DOMContentLoaded', onDomReady);
72 | } else {
73 | onDomReady();
at KThemePlugin (lib/KThemePlugin.js:70:5)
at Function.Vue.use (node_modules/vue/dist/vue.runtime.common.dev.js:5731:20)
at Object.use (jest.conf/setup.js:41:5)
at Object.require (jest.conf/visual.setup.js:1:1)
Test Suites: 1 failed, 1 total
|
a3f869f to
7802559
Compare
|
@Abhishek-Punhani As for the error, I haven't yet noticed anything in code that should be causing it. I will try on my end. Thanks for adding the examples. |
|
Hi @Abhishek-Punhani - I reviewed everything and very nice work. Thank you. As mentioned, I built on top of the documentation and tweaked it to represent Kolibri and Studio use-cases. I had two suggestions around how to (1) simplify inner styling, (2) make slot content's styling easier from consumers. However I weren't sure if they will work, so as I was finalizing documentation examples, I played around with some approaches and made those updates. I left notes on each change in commit messages so you still can receive some feedback. There was only one regression when the label was centered when using one of the inner slots. It seemed to be an intentional change to ensure no overlap with the label. I resolved it differently so that we don't need to center the label. Again see the commit message for details. Since we both did some significant changes of |
|
@Abhishek-Punhani actually I've just noticed one thing that I think was present before my changes too. If you'd like to look into why it's happening, that'd be great. Don't forget to pull my updates first though :)
And feel free to review the changes I pushed and let me know if you had any questions or noticed anything that I may have missed. |
Signed-off-by: Abhishek-Punhani <[email protected]>
It is not publicly exposed from KTextbox, neither used in consumers. To place icon before or after the input, the new outer/inner after/before slots can be used.
by not being opinionated which styles KDS applies to slots. Default styles are helpful in some cases, however overriding UiTextbox from consumers is difficult and we don't yet have sufficient amount of use-cases to see what's common patterns. Also don't render slot divs when slots are not being used.
and markup structure by removing unnecessary wrappers. Same effect can be achieved without them.
When inner slots were present, the label was centered, but it should be on the left side. Centered position was likely use to fix overlapping of the label with the content of the left inner slot in an empty input. This is now resolved by ensuring that the label is positioned in the upper area of the input (=not applying transform style) when an inner slot is present.
to make positioning of its content from consumers easier.
Example was moved to another file.
|
@MisRob , Thank you so much for the thorough review and for taking the time to build on top of the documentation. I really appreciate your guidance and feedback on this. |
Signed-off-by: Abhishek-Punhani <[email protected]>
339dcbc to
576d0a7
Compare
|
@MisRob , I noticed that on disabled and readonly conditions, a border is added to both |
|
Thanks @Abhishek-Punhani, yes I think the way you fixed the border makes sense. Before we merge, we will do regression testing in Kolibri. |
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 news @Abhishek-Punhani, we haven't observed any regressions.
Very nice work here, thanks a lot - this will not only make possible finishing your related Studio PR, but unblock many other tasks.
Also thanks for starting the documentation and visual tests. I know I tweaked those quite a bit, but it's much easier to do when there's some basis. This is what I do normally as I am trying to oversee the documentation quite carefully as a whole.
We will soon release a new version of KDS and install it in Studio. I will let you know when that's done.


Description
outerBefore,outerAfter,innerBefore,innerAfter) toKTextbox/UiTextboxfor flexible content injection before/after the input, both inside and outside the input area.Issue addressed
Fixes #1113
Before/after screenshots
Changelog
outerBefore,outerAfter,innerBefore,innerAfterslots toKTextboxto append elements before or after inputSteps to test
At a high level, how did you implement this?
Does this introduce any tech-debt items?
We will need to update the KDS website KtextBox docs
Testing checklist
Reviewer guidance
Comments