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

fix: adding overflow-wrap to dropdown #451 #482

Merged
merged 6 commits into from
Jan 23, 2020
Merged

fix: adding overflow-wrap to dropdown #451 #482

merged 6 commits into from
Jan 23, 2020

Conversation

bernhard-adobe
Copy link
Contributor

Adding overflow-wrap and hyphen option to dropdown label and limiting the width to 80% to force wrapping.

Description

In order to break "unbreakable words" like German connected nouns or urls / paths we need to allow overflow-wrap for words.

Fixes #451 (SDS-4649).
Previous fix was not sufficient int #452

How and where has this been tested?

  • How this was tested:

Swap out the following text in the components/dropdown/metadata/dropdown.yml
go tohttp://localhost:3000/docs/dropdown.html and inspect the dropdown label

  • Browser(s) and OS(s) this was tested with:

Browsers left to right:
On macOs Mojave 10.14.5 (18F203):
Chrome Version 81.0.4027.0 (Official Build) canary (64-bit)
Safari Version 12.1.1 (14607.2.6.1.1)
FireFox 72.0.1 (64-bit)
Edge Version 80.0.361.32 (Official build) Beta (64-bit)

On Windows 10:
Microsoft Edge 42.17134.1098.0

Screenshots

Screen Shot 2020-01-17 at 14 26 24

Screen Shot 2020-01-17 at 14 30 31

To-do list

  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • This pull request is ready to merge.

Copy link
Member

@lazd lazd left a comment

Choose a reason for hiding this comment

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

See the comment about width.

Also, can we test in IE 11 please?

word-break: break-all;
hyphens: auto;
overflow-wrap: break-word;
width: 80%;
Copy link
Member

Choose a reason for hiding this comment

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

80% is a bit arbitrary. Should this be calc(100% - whatever space we need for an icon)?

Copy link
Member

Choose a reason for hiding this comment

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

calc should be a good solution. I don't have IE 11 set up on my main machine. What solution are you using now for testing ie11? bootcamp?

Copy link
Member

Choose a reason for hiding this comment

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

@GarthDB I have a VM for it, Windows 8.1 IE 11, I believe it's this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Windows 8 instance is not supported anymore, it is recommended to use the Windows 10 instance that also has IE11 and MS Edge.
You can use VMWare fusion V11. (full setup here including serial number https://adobe.service-now.com/sc?id=kb_article&sys_id=1f47158d1bfd00d04bd1620f6e4bcbc4) or via SMB: smb://sjstore.corp.adobe.com/Labs/VMware/Fusion
You can download the latest supported Windows 10 here or ping me and I can send you the the fully configured VMWare fusion disk image (https://labvmlibrary.corp.adobe.com seems to be unresponsive at this time)

About using calc: It's only partially supported in IE11 (https://caniuse.com/#feat=calc) and will look like this in this scenario after gulp build (please ignore the background color, it's only demonstrating the width).

using width: calc(100% - (var(--spectrum-icon-checkmark-medium-width) + var(--spectrum-selectlist-option-icon-padding-x))); seems to work.

Screenshot in Chrome:
Screen Shot 2020-01-22 at 02 29 04

Screenshot in IE11:
Screen Shot 2020-01-22 at 02 28 54

Copy link
Member

Choose a reason for hiding this comment

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

@bernhard-adobe I'm aware that calc is partially supported in IE 11, and looking again at the known issues, none of them are relevant to our use case. Why did you bring it up? Is there something I missed? From your screenshot, it looks totally correct in IE11, right?

I'm not worried about the unsupported VM, it works perfectly fine, but thanks for the info!

Copy link
Contributor Author

@bernhard-adobe bernhard-adobe Jan 23, 2020

Choose a reason for hiding this comment

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

using width: calc(100% - (var(--spectrum-icon-checkmark-medium-width) + var(--spectrum-selectlist-option-icon-padding-x))); seems to work.

I mentioned earlier in my comment that I agreed to this testing of calc and it seems to work here.

Some known issues with calc in IE11 that could cause potential rendering issues (under "bugs" https://docs.w3cub.com/browser_support_tables/calc/):

  • unknown total width of the entire context menu, aka width is not set
  • auto-generated content gets set wrong width

@lazd
Copy link
Member

lazd commented Jan 22, 2020

@bernhard-adobe I went to test this in IE 11, but it seems your test strings from the screenshot are not present. Can you make another example or modify the current example so we can test this?

Copy link
Contributor Author

@bernhard-adobe bernhard-adobe left a comment

Choose a reason for hiding this comment

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

@lazd here is the HTML you can add the dropdown.yml.
I am not sure if we should add those corner cases to our components as it's kind of confusing to suddenly have different languages appear.

      <div class="spectrum-Dropdown is-open" style="width: 240px;">
        <button class="spectrum-FieldButton spectrum-Dropdown-trigger is-selected" aria-haspopup="listbox">
          <span class="spectrum-Dropdown-label">Donaudampfschifffahrtsgesellschaftskapitän</span>
          <svg class="spectrum-Icon spectrum-UIIcon-ChevronDownMedium spectrum-Dropdown-icon" focusable="false" aria-hidden="true">
            <use xlink:href="#spectrum-css-icon-ChevronDownMedium" />
          </svg>
        </button>
        <div class="spectrum-Popover spectrum-Popover--bottom spectrum-Dropdown-popover is-open" style="width: 100%">
          <ul class="spectrum-Menu" role="listbox">
            <li class="spectrum-Menu-item is-selected" role="option" aria-selected="true" tabindex="0">
              <span class="spectrum-Menu-itemLabel">Donaudampfschifffahrtsgesellschaftskapitän</span>
              <svg class="spectrum-Icon spectrum-UIIcon-CheckmarkMedium spectrum-Menu-checkmark spectrum-Menu-itemIcon" focusable="false" aria-hidden="true">
                <use xlink:href="#spectrum-css-icon-CheckmarkMedium" />
              </svg>
            </li>
            <li class="spectrum-Menu-item" role="option" tabindex="0">
              <span class="spectrum-Menu-itemLabel">Some crazy long value that should be cut off</span>
              <svg class="spectrum-Icon spectrum-UIIcon-CheckmarkMedium spectrum-Menu-checkmark spectrum-Menu-itemIcon" focusable="false" aria-hidden="true">
                <use xlink:href="#spectrum-css-icon-CheckmarkMedium" />
              </svg>
            </li>
            <li class="spectrum-Menu-item" role="option" tabindex="0">
              <span class="spectrum-Menu-itemLabel">A very long text that has spaces and hyphens-between-words.</span>
              <svg class="spectrum-Icon spectrum-UIIcon-CheckmarkMedium spectrum-Menu-checkmark spectrum-Menu-itemIcon" focusable="false" aria-hidden="true">
                <use xlink:href="#spectrum-css-icon-CheckmarkMedium" />
              </svg>
            </li>
            <li class="spectrum-Menu-divider" role="separator"></li>
            <li class="spectrum-Menu-item is-disabled" role="option" aria-disabled="true">
              <span class="spectrum-Menu-itemLabel">United States of America</span>
              <svg class="spectrum-Icon spectrum-UIIcon-CheckmarkMedium spectrum-Menu-checkmark spectrum-Menu-itemIcon" focusable="false" aria-hidden="true">
                <use xlink:href="#spectrum-css-icon-CheckmarkMedium" />
              </svg>
            </li>
          </ul>
        </div>
      </div>

Copy link
Member

@lazd lazd left a comment

Choose a reason for hiding this comment

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

The calc change looks good, but we need an example.

What I was saying is that we absolutely SHOULD add those examples for corner cases. It doesn't matter if it's confusing, these are our tests. If something breaks in the future with respect to word wrapping in dropdowns, we won't know unless we add those examples. Please add the example.

@bernhard-adobe
Copy link
Contributor Author

added examples.

Copy link
Member

@lazd lazd left a comment

Choose a reason for hiding this comment

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

After looking at the example, I tried adding it to the dropdown with the icon, and it doesn't fit:

image

@bernhard-adobe
Copy link
Contributor Author

thats the screenshot on Chrome Version 81.0.4027.0 (Official Build) canary (64-bit) and
Screen Shot 2020-01-22 at 17 07 05

Chrome Version 79.0.3945.117 (Official Build) (64-bit)
Screen Shot 2020-01-22 at 17 08 06

@bernhard-adobe
Copy link
Contributor Author

IE11 screenshot shows the checkmark:
Screen Shot 2020-01-22 at 17 12 50

@lazd
Copy link
Member

lazd commented Jan 23, 2020

@bernhard-adobe the issue I sent the screenshot of only happens when there is an icon, and I took it in Chrome. I have a fix for it, I can push it.

@lazd lazd merged commit 03208af into master Jan 23, 2020
@GarthDB GarthDB deleted the SDS-4649 branch September 21, 2020 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long words in Dropdown menu do not wrap
4 participants