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

feat(dialog): custom top position #1226

Merged
merged 9 commits into from Mar 15, 2022
Merged

feat(dialog): custom top position #1226

merged 9 commits into from Mar 15, 2022

Conversation

rachelbt
Copy link
Contributor

@rachelbt rachelbt commented Feb 2, 2022

No description provided.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

🚀

Latest successful build of the PR deployed here.

🚀

@rachelbt rachelbt linked an issue Feb 2, 2022 that may be closed by this pull request

:host([topPosition]) {
.mdc-dialog {
top: var(--dialog-top-position);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a wild card? do we want to maybe limit it to pre-defined tokens?

Copy link
Contributor Author

@rachelbt rachelbt Feb 10, 2022

Choose a reason for hiding this comment

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

do you mean this?:
$dialog-top-position: 100px;
top: var(#{$dialog-top-position}, --dialog-top-position);

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean be able to prevent arbitrary values. not sure what was the design decision eventually

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to 4 predefined sizes

Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

setting top position which exceeds the viewport boundaries will not be scrollable and the content wont be accessed.
if dialog isn't closed by clicking on scrim, the page will be stuck like that. this is a major flaw.

@jshenkman is this ok from a design perspective?

image

components/dialog/src/vwc-dialog.scss Show resolved Hide resolved
Comment on lines 84 to 95
:host([topPosition='small']) & {
--_top-position: 40px;
}
:host([topPosition='medium']) & {
--_top-position: 80px;
}
:host([topPosition='large']) & {
--_top-position: 120px;
}
:host([topPosition='xlarge']) & {
--_top-position: 160px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why aren't these interpolated from the const? as you defined above - #{$top-position}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still getting used to writing this this way :). I'll fix it

components/dialog/src/vwc-dialog.ts Outdated Show resolved Hide resolved
components/dialog/src/vwc-dialog.ts Outdated Show resolved Hide resolved
rachelbt and others added 3 commits February 22, 2022 13:24
Co-authored-by: yinon <yinon@hotmail.com>
Co-authored-by: yinon <yinon@hotmail.com>
@jshenkman
Copy link
Contributor

setting top position which exceeds the viewport boundaries will not be scrollable and the content wont be accessed. if dialog isn't closed by clicking on scrim, the page will be stuck like that. this is a major flaw.

@jshenkman is this ok from a design perspective?

image

No, it's not a good experience if the user can't see the whole modal and the actions/footer..
Can we add a scrolling option to the modal in this case?

@yinonov
Copy link
Contributor

yinonov commented Feb 23, 2022

Can we add a scrolling option to the modal in this case?

No. It's unaware

@sonarcloud
Copy link

sonarcloud bot commented Mar 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rachelbt rachelbt merged commit ad95c7b into master Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Dialog] add support for position-top
3 participants