Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issues #311 and #313 #315

Merged
merged 3 commits into from
Mar 8, 2024
Merged

Fix issues #311 and #313 #315

merged 3 commits into from
Mar 8, 2024

Conversation

gnbm
Copy link
Collaborator

@gnbm gnbm commented Mar 7, 2024

This PR aims to fix two issues:

What was happening

  1. When using dropdown with options shown as tags where the options have HTML content, the display of the tags gets broken as we can see in the following image (this was introduced in v1.0.41):

image

  1. When using dropdown with settings including allowNewOption:true with an empty list of options caused the search input to be disabled (this was introduced in v1.0.41):

image

After the fix

  • Included a new section to exemplify the scenario where a dropdown with options shown as tags with the options having HTML content - ✅

image

  • Followed the same steps above - ✅
  • Ran regression scenarios - ✅
  • Run automated tests - ✅

image

Both are now working as expected:

//Will cause text overflow in runtime and if so, the tooltip information is prepared
if(Utils.willTextOverflow($valueText.parentElement, label))
{
const valueTooltipForTags = this.getTooltipAttrText(label, false, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be defined as empty, and inside the if it will have the expected value, then assigned into the valueTagHtml string, if it's empty it will add nothing, otherwise it will add the expected attribute. This way the if/else can be removed ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joselrio Don't know why but the condition was not committed. Thank you for raising that 💪🏻

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joselrio Both changes are now committed. Thank you for the great review 🚀

@joselrio
Copy link
Contributor

joselrio commented Mar 8, 2024

Also @gnbm by looking into the file you're PullRequested I think there's a missing validation to fix the issue about the search input disabled when allowNewOption=true ;)

@gnbm gnbm requested a review from joselrio March 8, 2024 18:38
@gnbm gnbm merged commit a89cf25 into sa-si-dev:master Mar 8, 2024
@gnbm gnbm mentioned this pull request Mar 10, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants