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.

Sign up for GitHub

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

Jump to bottom

fix: (Core) issues on Popover related components #3723

Merged
merged 13 commits into from
Nov 10, 2020
Merged

fix: (Core) issues on Popover related components #3723

merged 13 commits into from
Nov 10, 2020

Conversation

JKMarkowski
Copy link
Contributor

@JKMarkowski JKMarkowski commented Nov 2, 2020 •
edited

Please provide a link to the associated issue.

Closes #3694

Please provide a brief summary of this pull request.

There are

  • changes mostly in select/combobox components regarding width of control and body. + Overflow for components with truncation.
  • fix for IE11 for multiinput - for some reason focusing input was opening multi input. which made using multiinput impossible
  • datetime picker width change
  • fix for bug with height in some components

Before:
Combobox:
image

DateTimePicker:
image

Select:
image

After:
Combobox:
image

DateTimePicker:
image

Select:
image

Please check whether the PR fulfills the following requirements

@JKMarkowski JKMarkowski requested a review from a team November 2, 2020 13:06
@netlify
Copy link

netlify bot commented Nov 2, 2020 •
edited

Deploy preview for fundamental-ngx ready!

Built with commit 54b65f6

https://deploy-preview-3723--fundamental-ngx.netlify.app

@JKMarkowski JKMarkowski self-assigned this Nov 2, 2020
@JKMarkowski JKMarkowski added the core Core library specific issues label Nov 2, 2020
@JKMarkowski JKMarkowski added this to In progress in Development via automation Nov 2, 2020
@JKMarkowski JKMarkowski added this to the Sprint 49 - Nassau milestone Nov 2, 2020
@InnaAtanasova InnaAtanasova changed the title WIP fix: apply proper width to popover related components [WIP] fix: apply proper width to popover related components Nov 2, 2020
JKMarkowski
JKMarkowski commented Nov 2, 2020
View reviewed changes
...re/component-docs/select/examples/select-nested-options/select-nested-options.component.html
@@ -1,4 +1,5 @@
<fd-select [(value)]="selectedValue"
[extendedBodyTemplate]="true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extendedBodyTemplate is added to let Option Components know if the text should be wrapped in fd-list-title directive. It's mostly to keep overflow in correct way

@JKMarkowski JKMarkowski changed the title [WIP] fix: apply proper width to popover related components fix: apply proper width to popover related components Nov 2, 2020
stefanoScalzo
stefanoScalzo previously requested changes Nov 3, 2020
View reviewed changes
libs/core/src/lib/select/select.component.scss
}
}
.fd-popover__body > :first-child {
border-radius: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

will also need to fix the border radius of the popover

libs/core/src/lib/combobox/combobox.component.ts Outdated
* Preset options for the Combobox body width.
* * `at-least` will apply a minimum width to the body equivalent to the width of the InputGroup. - Default
* * `equal` will apply a width to the body equivalent to the width of the InputGroup.
* * '' for no effect
Copy link
Contributor

Choose a reason for hiding this comment

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

Describe what is meant by no effect please

@JKMarkowski
Copy link
Contributor Author

@stefanoScalzo Your comments has been addresed, can you check it again?

@JKMarkowski JKMarkowski mentioned this pull request Nov 4, 2020
Closed
@JKMarkowski JKMarkowski requested a review from a team November 5, 2020 08:40
@JKMarkowski JKMarkowski changed the title fix: apply proper width to popover related components fix: (core) issues on Popover related components Nov 5, 2020
InnaAtanasova
InnaAtanasova approved these changes Nov 5, 2020
View reviewed changes
libs/core/src/lib/select/option/option.component.ts Outdated
Comment on lines 73 to 74
/** @hidden Whether option contains more than basic text. */
extendedTemplate = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's hidden it should be prefixed with _ as we discussed

Copy link
Contributor

Choose a reason for hiding this comment

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

You can wait till we do the refactoring as this is not a new component.

@InnaAtanasova
Copy link
Contributor

Are these related to the PR? This is Select component
Screen Shot 2020-11-05 at 11 36 09 AM
Screen Shot 2020-11-05 at 11 36 03 AM

@JKMarkowski
Copy link
Contributor Author

JKMarkowski commented Nov 5, 2020 •
edited

Are these related to the PR? This is Select component
Screen Shot 2020-11-05 at 11 36 09 AM

This one was there for a long time. I guess container for message fd-form-message should be shorter, but it's definetely scope of this PR.

salarenko
salarenko suggested changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@salarenko salarenko left a comment

Choose a reason for hiding this comment

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

  • Select in mobile mode still takes all available width
    image

  • I couldn't find any straight information, but I think according to the documentation Multi Input should not be growing when tokens are added.
    growing-multi-input

libs/core/src/lib/combobox/combobox.component.ts Show resolved Hide resolved
libs/core/src/lib/combobox/combobox.component.html Show resolved Hide resolved
libs/core/src/lib/multi-input/multi-input.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/multi-input/multi-input.component.ts Show resolved Hide resolved
libs/core/src/lib/popover/popover-position/popover-position.ts Outdated Show resolved Hide resolved
libs/core/src/lib/popover/popover.component.scss Outdated Show resolved Hide resolved
libs/core/src/lib/popover/popover.component.ts Show resolved Hide resolved
@JKMarkowski JKMarkowski changed the title fix: (core) issues on Popover related components fix: (Core) issues on Popover related components Nov 6, 2020
@kavya-b kavya-b mentioned this pull request Nov 10, 2020
Merged
5 tasks
@JKMarkowski JKMarkowski merged commit b2a62b3 into master Nov 10, 2020
Development automation moved this from In progress to Done Nov 10, 2020
@JKMarkowski JKMarkowski deleted the fix/popover-related-components branch November 10, 2020 11:54
@kavya-b kavya-b mentioned this pull request Dec 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@InnaAtanasova InnaAtanasova InnaAtanasova approved these changes

@salarenko salarenko salarenko approved these changes

@stefanoScalzo stefanoScalzo Awaiting requested review from stefanoScalzo

Assignees

@JKMarkowski JKMarkowski

Labels
core Core library specific issues Defect Hunting
Projects
No open projects
Development
  
Done
Milestone
Sprint 50 - Tulum
Development

Successfully merging this pull request may close these issues.

Popover Related Components - Width
4 participants
@JKMarkowski @InnaAtanasova @salarenko @stefanoScalzo