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

made KTooltip's width configurable #611

Merged
merged 8 commits into from
Apr 15, 2024
Merged

made KTooltip's width configurable #611

merged 8 commits into from
Apr 15, 2024

Conversation

satyamkale27
Copy link
Contributor

@satyamkale27 satyamkale27 commented Apr 9, 2024

Description

This PR configures the maxWidth of KTooltip, If maxWidthis not passed it will maintain its current behavior of adjusting its size to fit its content

Issue addressed

#595

Addresses #PR# HERE

screenshots

If maxWidth is not passed, KTooltip will maintain its current behavior of adjusting its size to fit its content.
before

If maxWidth passes.
after

Changelog

  • [#PR no]
    • Description: This PR configures the maxWidthof KTooltip, If maxWidth is not passed it will maintain its current behavior of adjusting its size to fit its content
    • Products impact:
    • Addresses: Make KTooltip's width configurable  #595
    • Components: -
    • Breaking: no
    • Impacts a11y: -
    • Guidance: -

[#PR no]: PR link

Steps to test

  1. Step 1
    Run the KDS By yarn dev
  2. Step 2
    Past this code snippet to the playground.vue file in following path docs/pages /playground.vue
<KIcon ref="icon" icon="info" />
    <KTooltip
      :maxWidth="'100px'"
      reference="icon"
      :refs="$refs"
    >
      Tooltip text tooltip text tooltip text tooltip text tooltip text
    </KTooltip>
  1. Past this in your browser http://localhost:4000/playground and see the changes

(optional) Implementation notes

At a high level, how did you implement this?

Briefly describe how this works

If the maxWidth is passed,the maxWidth will get applied. but if maxWidth is not passed it will continue its current behavior of adjusting its size to fit its content.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?

After review

  • The changelog item has been pasted to the CHANGELOG.md

Comments

@satyamkale27
Copy link
Contributor Author

@MisRob please take a look.

@MisRob
Copy link
Member

MisRob commented Apr 9, 2024

Thanks @satyamkale27, as mentioned in the issue I think this implementation is an option if the preferred API

<KTooltip
  :style="{ 'max-width': '100px' }"
  reference="icon"
  :refs="$refs"
>
  Tooltip text tooltip text tooltip text tooltip text tooltip text
</KTooltip>

is not feasible for some reason.

Could you elaborate if you've explored it and what were the blockers?

@satyamkale27
Copy link
Contributor Author

satyamkale27 commented Apr 9, 2024

@MisRob I have not yet explored an alternative approach to it, but I could. correct me if I am wrong, means we don't want max-width set inline like here in this code snippet
<KIcon ref="icon" icon="info" /> <KTooltip :maxWidth="'100px'" reference="icon" :refs="$refs" > Tooltip text tooltip text tooltip text tooltip text tooltip text </KTooltip> please explain me in brief so i can work on it.

@MisRob
Copy link
Member

MisRob commented Apr 9, 2024

Thank you @satyamkale27. The difference between the two options is that in the first one I suggest we explore first, max width is set via the standard CSS max-width, for example with :style="{ 'max-width': '100px' }". The question here is - if you copy the code from the "Current behavior" of #595 to the playground, is there something we can do inside KTooltip to make it work so that we don't even need the new maxWidth property?

If it doesn't seem to be feasible, we could add a new prop maxWidth for sure, but if it is possible, I would say that using usual style is preferable.

@satyamkale27
Copy link
Contributor Author

thanks for explaning I got it now, soon I will make new commit.

@MisRob MisRob self-requested a review April 10, 2024 07:03
@MisRob MisRob self-assigned this Apr 10, 2024
@satyamkale27
Copy link
Contributor Author

@MisRob I made a new commit which will allow max-width to run but there is a problem in it, i cant use the style prop because "style" is a reserved attribute and cannot be used as component prop. so i used styles to pass prop.

@MisRob
Copy link
Member

MisRob commented Apr 10, 2024

Hey, @satyamkale27, thanks for following up.

"style" is a reserved attribute and cannot be used as component prop.

Ah yes, I didn't mean you use it as a component prop, but rather as ordinary native inline style attribute. Vue syntax

<KTooltip
  :style="{ 'max-width': '100px' }"
  reference="icon"
  :refs="$refs"
>
  Tooltip text tooltip text tooltip text tooltip text tooltip text
</KTooltip>

will translate into native HTML inline style="max-width:100px;" and my suggestions was to explore whether we can simply use it to achieve the desired result rather than adding a new prop. For example by experimenting with adjusting internal styles of the KTooltip so that they inherit/respect the root container style width settings and/or perhaps with attaching v-bind:style on one of the children elements in KTooltip.

A simple parallel would be, let's say we'd have two nested divs

<div style="max-width:100px;">
  <div class="some-styles">
    Tooltip text tooltip text tooltip text tooltip text tooltip text
  </div>
</div>

and now the task is to investigate what about the styles applied by some-styles class cause that the inner div doesn't accept the outer div's max width settings.

Note that this doesn't necessarily reflect KTooltips internals exactly, I'm just using it as a simplified example. I think that if we understood more what's happening in this regard, it will help to inform next steps.

Please let me know if this helped to clarify.

If you won't find a way to achieve so in this manner, we can return to the maxWidth option, I just think it'd be useful to see whether this direction is feasible or not and why.

@satyamkale27
Copy link
Contributor Author

@MisRob thanks for your guidance, I have explored many methods to achive without passing prop , but none of them worked.

@MisRob
Copy link
Member

MisRob commented Apr 11, 2024

Alright, thanks @satyamkale27, I will take a look

@MisRob MisRob added the TODO: needs review Waiting for review label Apr 11, 2024
@MisRob
Copy link
Member

MisRob commented Apr 11, 2024

@satyamkale27 I played around with it too a bit and like you, I haven't really discovered a way to achieve the first option in a reasonable time frame due to Popper. Thanks for your time investigating. Let's go with the maxWidth prop then.

@satyamkale27
Copy link
Contributor Author

@MisRob yeh sure, I will make a new commit soon.

@MisRob
Copy link
Member

MisRob commented Apr 12, 2024

Thanks @satyamkale27, I think that just reverting to d82513c will work

@satyamkale27
Copy link
Contributor Author

satyamkale27 commented Apr 12, 2024

@MisRob I made a revert please take a look. in this case we can only use maxWidth not other styles, but if we pass styles prop as a object we can use other styles like color and so on, also including "max-width" as i done in previous commit 611,
so If we dont want styles as prop I have made revert already which will allow to use maxWidth

@MisRob
Copy link
Member

MisRob commented Apr 12, 2024

Thank you @satyamkale27. You're right that styles prop would be more general and has a benefit of allowing for other customizations. Right now, I lean towards maxWidth since when native Web APIs are not possible to use, we tend to use restricted Vue component API to promote consistent experience tailored to our use-cases. Also specifically for the tooltip, it seems that over the years there haven't been much need for other customizations. So that's what behind the decision, however it's just one viewpoint, and each approach has its pros and cons. In the future, if we find out we need more flexibility, we can consider styles prop certainly. For now we can leave the PR as is and I will finish the review next week.

@satyamkale27
Copy link
Contributor Author

satyamkale27 commented Apr 12, 2024

@MisRob yeh, thats good thank you.

Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Thank you, @satyamkale27, all good.

Before merging, I updated the changelog file and also documented the new prop because this section of the documentation page is auto-generated from the in-code docstrings

Screenshot from 2024-04-15 05-39-54

@MisRob MisRob merged commit 3c97fac into learningequality:release-v3 Apr 15, 2024
8 checks passed
@satyamkale27
Copy link
Contributor Author

Thankyou that's look good.

@MisRob MisRob mentioned this pull request Apr 15, 2024
1 task
@satyamkale27 satyamkale27 deleted the KTooltip-maxWidth branch April 23, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants