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

Add {{page-title}} to route templates #19224

Merged
merged 1 commit into from
Oct 24, 2020
Merged

Add {{page-title}} to route templates #19224

merged 1 commit into from
Oct 24, 2020

Conversation

raido
Copy link
Contributor

@raido raido commented Oct 23, 2020 •
edited
Loading

Changes in this PR are for ember-page-title RFC - ember-cli/ember-cli#9370

Related PR for ember-cli: ember-cli/ember-cli#9372

TODO

  • detect if the app has ember-page-title installed, and only add the {{page-title ...}} invocation

@raido raido mentioned this pull request Oct 23, 2020
Merged
rwjblue
rwjblue requested changes Oct 23, 2020
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Can you add some logic to the blueprint to detect if the app has ember-page-title installed, and only add the {{page-title ...}} invocation when it is?

@raido raido force-pushed the add-ember-page-title branch from 322bc32 to 578f450 Compare October 23, 2020 13:25
@raido
Copy link
Contributor Author

raido commented Oct 23, 2020

Can you add some logic to the blueprint to detect if the app has ember-page-title installed, and only add the {{page-title ...}} invocation when it is?

Sure, first I am just trying to get current test suite green and then add additional logic :)

@raido raido force-pushed the add-ember-page-title branch from 578f450 to c374f96 Compare October 23, 2020 13:31
@raido
Copy link
Contributor Author

raido commented Oct 23, 2020 •
edited
Loading

Failed with some weird TypeError - https://github.com/emberjs/ember.js/pull/19224/checks?check_run_id=1298266214#step:7:109 - not sure if related or not @rwjblue ?

@raido raido force-pushed the add-ember-page-title branch from c374f96 to f32e33d Compare October 23, 2020 16:05
raido
raido commented Oct 23, 2020
View reviewed changes
blueprints/route/files/__root__/__templatepath__/__templatename__.hbs
<% if (addTitle) {%>{{page-title "<%= routeName %>"}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Syntax is like this to keep whitespace consistent.

@raido raido force-pushed the add-ember-page-title branch from f32e33d to baf62f5 Compare October 23, 2020 16:15
@raido raido marked this pull request as ready for review October 23, 2020 16:33
@raido
Copy link
Contributor Author

raido commented Oct 23, 2020

CI failure seems to be same with master branch.

@raido raido requested a review from rwjblue October 23, 2020 16:33
@raido
Copy link
Contributor Author

raido commented Oct 23, 2020

Actually, I am tempted to reverse test setup logic. So all previous tests would remain unchanged and "ember-page-title installed" cases are added. It is then much less changes.

@rwjblue
Copy link
Member

rwjblue commented Oct 23, 2020

I believe #19225 will resolve that.

@raido raido force-pushed the add-ember-page-title branch from baf62f5 to ed56eeb Compare October 23, 2020 18:49
@raido
Copy link
Contributor Author

raido commented Oct 23, 2020

@rwjblue Rebased on master, CI green and tests refactored.

@rwjblue rwjblue merged commit 7b95155 into emberjs:master Oct 24, 2020
@rwjblue
Copy link
Member

rwjblue commented Oct 24, 2020

Thanks @raido!

@raido raido deleted the add-ember-page-title branch October 25, 2020 04:16
@locks locks mentioned this pull request Jan 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@raido @rwjblue