Skip to content

Add bottom navigation title deprecation migration docs - #4412

Merged
sfshaza2 merged 9 commits into
flutter:masterfrom
johnsonmh:bottom_nav_breaking_change
Sep 24, 2020
Merged

Add bottom navigation title deprecation migration docs#4412
sfshaza2 merged 9 commits into
flutter:masterfrom
johnsonmh:bottom_nav_breaking_change

Conversation

@johnsonmh

@johnsonmh johnsonmh commented Jul 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Jul 31, 2020
@johnsonmh
johnsonmh requested a review from sfshaza2 July 31, 2020 16:05
@johnsonmh johnsonmh mentioned this pull request Jul 31, 2020
Merged
12 of 13 tasks
rami-a
rami-a approved these changes Jul 31, 2020
View reviewed changes
Comment thread src/docs/release/breaking-changes/bottom-navigation-title-to-label.md Outdated
Comment thread src/docs/release/breaking-changes/bottom-navigation-title-to-label.md Outdated
Comment thread src/docs/release/breaking-changes/bottom-navigation-title-to-label.md Outdated

## Summary

{% comment %}

@rami-a rami-a Jul 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure if these are meant to be removed or if they'll be invisible

@sfshaza2 sfshaza2 Jul 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They'll be invisible.

@sfshaza2 sfshaza2 Jul 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While the comments are invisible, can you remove them from the file? They just add clutter.

@johnsonmh johnsonmh Sep 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

sfshaza2
sfshaza2 requested changes Jul 31, 2020
View reviewed changes

@sfshaza2 sfshaza2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to fix the build and add timeline info before I can land.

Comment thread src/docs/release/breaking-changes/bottom-navigation-title-to-label.md Outdated

## Summary

{% comment %}

@sfshaza2 sfshaza2 Jul 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While the comments are invisible, can you remove them from the file? They just add clutter.

Comment thread src/docs/release/breaking-changes/bottom-navigation-title-to-label.md Outdated

## Timeline

Landed in version:

@sfshaza2 sfshaza2 Jul 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can't publish this PR until we have a version #.

@johnsonmh johnsonmh Sep 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added 1.22.0!

Comment thread src/docs/release/breaking-changes/index.md Outdated
[The Route Transition record and Transition delegate updates]: /docs/release/breaking-changes/route-transition-record-and-transition-delegate
[ThemeData's accent properties]: /docs/release/breaking-changes/theme-data-accent-properties
[Material Chip button semantics]: /docs/release/breaking-changes/material-chip-button-semantics
[Bottom Navigation Item Title]: /docs/release/breaking-changes/bottom-navigation-title-to-label.md

@sfshaza2 sfshaza2 Jul 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Delete the ".md" extension. The system converts markdown files to a non-markdown (HTML) format, though we don't use those extensions eithre. This is what is currently breaking the Travis build, btw.

@johnsonmh johnsonmh Sep 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, thanks for spotting that.

sfshaza2
sfshaza2 requested changes Sep 22, 2020
View reviewed changes

@sfshaza2 sfshaza2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall, looks pretty good! I do have a couple suggested changes, then I can land it..

Comment thread src/docs/release/breaking-changes/bottom-navigation-title-to-label.md Outdated
## Description of change

The `BottomNavigationBarItem` class has a `title` parameter, which is a `Widget`. This made it
impossible for the `BottomNavigationBar` to show `Tooltip` widgets, a change which was necessary to

@sfshaza2 sfshaza2 Sep 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

which was necessary => that was necessary

@johnsonmh johnsonmh Sep 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread src/docs/release/breaking-changes/bottom-navigation-title-to-label.md Outdated

The `BottomNavigationBarItem` class has a `title` parameter, which is a `Widget`. This made it
impossible for the `BottomNavigationBar` to show `Tooltip` widgets, a change which was necessary to
improve the accessibility experience. Now, instead of taking `BottomNavigationBarItem.title` widget

@sfshaza2 sfshaza2 Sep 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

=>
Now, instead of building the BottomNavigationBarItem.title widget, the BottomNavigationBar wraps the BottomNavigationBarItem.label in a Text widget and builds that.

@johnsonmh johnsonmh Sep 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

sfshaza2
sfshaza2 approved these changes Sep 24, 2020
View reviewed changes

@sfshaza2 sfshaza2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@sfshaza2
sfshaza2 merged commit 8887033 into flutter:master Sep 24, 2020
filiph pushed a commit to filiph/website that referenced this pull request Feb 22, 2021
@johnsonmh @rami-a @filiph
Add bottom navigation title deprecation migration docs (flutter#4412)
c7fff2a 
* Add bottom navigation title deprecation migration docs

* Update src/docs/release/breaking-changes/bottom-navigation-title-to-label.md

Co-authored-by: Rami <[email protected]>

* Update src/docs/release/breaking-changes/bottom-navigation-title-to-label.md

Co-authored-by: Rami <[email protected]>

* Shams comments

* remove comments

* finish merge

* fix formatting

* more shams suggestions

Co-authored-by: Rami <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sfshaza2 sfshaza2 sfshaza2 approved these changes
+1 more reviewer
@rami-a rami-a rami-a approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes Contributor has signed the Contributor License Agreement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@johnsonmh @rami-a @sfshaza2 @googlebot