Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

oliversalzburg
Copy link
Contributor

If the user specifies no position at all, the toast will open from the top, while the class name will reflect a toast opening from the bottom.

By chaning the ternary as suggested, we end up with the correct default case while maintaining all configured cases as-is.

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Go to https://material.angularjs.org/latest/demo/toast and uncheck all 4 checkboxes, then trigger a toast. The toast will appear on the top, the bottom FAB will make space for the toast.

What is the new behavior?

The toast will appear on the top, the top FAB will make space for the toast.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jan 15, 2020
@Splaktar
Copy link
Contributor

Splaktar commented Feb 1, 2020

From https://material.angularjs.org/latest/api/service/$mdToast#mdtoast-show-optionsorpreset:

position - {string=}: Sets the position of the toast.
Available: any combination of 'bottom', 'left', 'top', 'right', 'end', and 'start'. The properties 'end' and 'start' are dynamic and can be used for RTL support.

Default combination: 'bottom left'.

I managed to fix the example by not passing "" into .position() as that is an invalid value. However, the code should probably handle the empty string input by using the default "bottom left" instead of having it somehow result in "top left".

@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Feb 1, 2020
@Splaktar Splaktar changed the title fix(toast): FAB misaligned with empty position fix(toast): toast defaults to wrong values when passed an empty position Feb 1, 2020
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Can we try to have the function that handles the position input use the proper, documented defaults when an empty string or any other invalid value is passed in?

@oliversalzburg
Copy link
Contributor Author

Good point. I will submit an updated version next week.

@Splaktar
Copy link
Contributor

Sounds good.

If no position was provided for whatever reason, the default behavior
should be to open from the bottom.
@oliversalzburg
Copy link
Contributor Author

@Splaktar Sorry for the delay. It should be better now.

@Splaktar Splaktar added this to the 1.1.23 milestone May 4, 2020
@Splaktar Splaktar self-assigned this May 4, 2020
@Splaktar Splaktar removed this from the 1.1.23 milestone May 5, 2020
Splaktar added a commit that referenced this pull request May 5, 2020
- position of "" uses document default positioning
  - "bottom left"
- position of "left" or "right" uses default vertical positioning: "bottom"
- improve JSDoc

Closes #11843
Splaktar added a commit that referenced this pull request May 5, 2020
- position of "" uses documented default positioning
  - "bottom left"
- position of "left" or "right" uses default vertical positioning: "bottom"
- improve JSDoc

Closes #11843
@Splaktar
Copy link
Contributor

Splaktar commented May 5, 2020

I tested this updated fix and it doesn't appear to work.

After some investigation, I discovered that there was actually a different place in the code that needed to change. I've opened PR #11904 to solve this.

Splaktar added a commit that referenced this pull request Jun 29, 2020
- position of "" uses documented default positioning
  - "bottom left"
- position of "left" or "right" uses default vertical positioning: "bottom"
- improve JSDoc

Closes #11843.

BREAKING CHANGE: $mdToast.show()'s position behavior has been updated to be consistent with the documentation. If you relied on the previously undocumented behavior where it defaulted to `top left` instead of `bottom left`, you will need to update your app.

Change your code from this:

```js
    $mdToast.show(
      $mdToast.simple()
      .textContent('Simple Toast!'))
    .then(...
```

To this:

```js
    $mdToast.show(
      $mdToast.simple()
      .textContent('Simple Toast!')
      .position('top left'))
    .then(...
```
Splaktar added a commit that referenced this pull request Jun 29, 2020
- position of "" uses documented default positioning
  - "bottom left"
- position of "left" or "right" uses default vertical positioning: "bottom"
- improve JSDoc

Closes #11843.

BREAKING CHANGE: `$mdToast.show()`'s position behavior has been updated to be consistent with the documentation. If you relied on the previously undocumented behavior where it defaulted to `top left` instead of `bottom left`, you will need to update your app.

Change your code from this:

```js
    $mdToast.show(
      $mdToast.simple()
      .textContent('Simple Toast!'))
    .then(...
```

To this:

```js
    $mdToast.show(
      $mdToast.simple()
      .textContent('Simple Toast!')
      .position('top left'))
    .then(...
```
Splaktar added a commit that referenced this pull request Jun 29, 2020
- position of "" uses documented default positioning
  - "bottom left"
- position of "left" or "right" uses default vertical positioning: "bottom"
- improve JSDoc

Closes #11843.

BREAKING CHANGE: `$mdToast.show()`'s position behavior has been updated to be consistent with the documentation. If you relied on the previously undocumented behavior where it defaulted to `top left` instead of `bottom left`, you will need to update your app.

Change your code from this:

```js
    $mdToast.show(
      $mdToast.simple()
      .textContent('Simple Toast!'))
    .then(...
```

To this:

```js
    $mdToast.show(
      $mdToast.simple()
      .textContent('Simple Toast!')
      .position('top left'))
    .then(...
```
@Splaktar Splaktar added this to the 1.2.0 milestone Jun 29, 2020
@Splaktar Splaktar added resolution: fixed P3: important Important issues that really should be fixed when possible. type: bug and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Jun 29, 2020
@Splaktar Splaktar removed this from the 1.2.0 milestone Jun 29, 2020
@Splaktar Splaktar added this to the 1.2.0 milestone Jun 29, 2020
@oliversalzburg oliversalzburg deleted the fix/toast-default-alignment branch July 13, 2020 09:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P3: important Important issues that really should be fixed when possible. type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants