Skip to content
This repository has been archived by the owner on Mar 29, 2023. It is now read-only.

Added Modal Component #38

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Added Modal Component #38

wants to merge 10 commits into from

Conversation

mathis-m
Copy link
Contributor

Added modal with popup branch merged

@mathis-m
use elvis operator
3df336f
@mathis-m
Merge branch 'feature/component/popup' into feature/component/modal
b1ae9fa 
# Conflicts:
#	packages/documentation/Overview.vue
#	src/components/index.ts
@mathis-m
add "OfficeModal"
d09e94c 
and

Merge branch 'feature/component/popup' into feature/component/modal

# Conflicts:
#	packages/documentation/Overview.vue
#	src/components/index.ts

TODO: remove v-if must have for working OfficeModal and Popup
@s-bauer s-bauer changed the title Feature/component/modal Added Modal Component Jan 13, 2019
@s-bauer s-bauer added enhancement New feature or request new component A new Office Fabric Component labels Jan 13, 2019
@s-bauer
Copy link
Owner

s-bauer commented Jan 13, 2019 •
edited
Loading

  • isClickableOutsideFocusTrap and isBlocking is redundant. It's only included in the react library due to the common interface IAccessiblePopupProps. Let's just keep isBlocking and remove the other one.
  • Closing animation missing
  • Unable to close popup by clicking outside of it
  • Don't use function props like onDismiss. Use an event! (``$emit("dismiss")`)
  • You removed containerClassName and scrollableContentClassName and therefore lost the functionality to style the inner components. That's why you need these ugly flex and center properties. Get rid of them and add back the two ...ClassName properties!
  • The FocusTrapZone is missing a :class="classNames.main". Also the custom style is wrong, as the FocusTrapZone should not be 100% height and width. Compare with https://developer.microsoft.com/en-us/fabric#/components/modal
  • OfficeOverlay has a "visible" property now, which you did not set, so it's always false.
  • Your state property makes no sense. A state can never be a getter only! Just use normal class fields for that kind of stuff

s-bauer and others added 4 commits January 13, 2019 20:00
@s-bauer
Merge branch 'master' into feature/component/modal
753c710 
# Conflicts:
#	packages/documentation/Overview.vue
#	packages/office-ui-fabric-vue/components/Popup/OfficePopup.vue
#	src/components/index.ts
@mathis-m
remove isBlocking redundancy
fcc24f7 
fix isBlocking logic (onDismiss emit on click outside)
added classnames props for all configurable not root elements
in FocusTrapZone added missing :class="classNames.main"
OfficeOverlay now with visable prop.(fixed on destroy enable scroll)
@mathis-m
refactor
efda5b6
@mathis-m
Merge remote-tracking branch 'upstream/master' into feature/component…
80eea33 
…/modal
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement New feature or request new component A new Office Fabric Component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mathis-m @s-bauer