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(config): expose getMode() and deprecate Config #19104

Merged
merged 4 commits into from Sep 25, 2019

Conversation

manucorporat
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

@ionitron-bot ionitron-bot bot added package: angular @ionic/angular package package: core @ionic/core package labels Aug 14, 2019
angular/src/providers/config.ts Outdated Show resolved Hide resolved
@@ -199,3 +199,16 @@ export const setupConfig = (config: IonicConfig) => {
};
return win.Ionic.config;
};

export const getMode = (): Mode => {
const win = window as any;
Copy link
Member

Choose a reason for hiding this comment

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

should this be IonicWindow as well?

Co-Authored-By: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
@masimplo
Copy link
Contributor

masimplo commented Oct 3, 2019

Hi, any info on why config.set was deprecated and what should be used instead? The deprecation message does not offer an alternative.

I am using md config irrespective of platform but want to set rippleEffect to false when the platform is iOS - which I am currently doing using config.set.

 this._config.set('rippleEffect', this._platform.is('android'));

@liamdebeasi
Copy link
Member

You can still set the config in app.module.ts: https://ionicframework.com/docs/utilities/config#basic-example

@masimplo
Copy link
Contributor

masimplo commented Oct 4, 2019

You can still set the config in app.module.ts: https://ionicframework.com/docs/utilities/config#basic-example

Yes, but from what I understand a value has to be hard-coded (either true or false for both platforms) as you don't have access to the platform service then. Unless there is a different way to differentiate between platforms that I am unaware of.

@liamdebeasi
Copy link
Member

liamdebeasi commented Oct 4, 2019

You can probably just test the user agent directly in there to get the same result using a regex: /android|sink/i.test(window.navigator.userAgent)

(this is the same regex we use in the platform service)

@chriswep
Copy link

chriswep commented Oct 4, 2019

Hi, any info on why config.set was deprecated and what should be used instead? The deprecation message does not offer an alternative.

I also have this question. I currently change backButtonText on language change, triggered by the user on runtime. How should this be done in the future @liamdebeasi ?

@JesperBalslev
Copy link

I also have this question. I currently change backButtonText on language change, triggered by the user on runtime. How should this be done in the future @liamdebeasi ?

I would like to know the best practice for this use case also. @liamdebeasi

@liamdebeasi
Copy link
Member

@chriswep How are you using it currently? You should be able to set it in IonicModule.forRoot.

@chriswep
Copy link

@liamdebeasi It’s a use case in my app to change the language at runtime.

@liamdebeasi
Copy link
Member

Right, my question was more of how you are currently doing it code-wise.

@chriswep
Copy link

Right, my question was more of how you are currently doing it code-wise.

after the user has changed the language and after the new language data is ready i do

this.config.set('backButtonText', this.translate.instant('common.button.back'));

with this.config being the Ionic Angular Config service.

@liamdebeasi
Copy link
Member

Chatting about this with the team. Will get back to you!

@JesperBalslev
Copy link

Right, my question was more of how you are currently doing it code-wise.

after the user has changed the language and after the new language data is ready i do

this.config.set('backButtonText', this.translate.instant('common.button.back'));

with this.config being the Ionic Angular Config service.

The exact same here, using it like this:

this.config.set('backButtonText', this.translate.instant('GENERAL.back'));

@swbradsh
Copy link

swbradsh commented Nov 7, 2019

I'm also using config.set to do custom animations between certain pages - not all pages. I didn't see any other way to do this.

this.config.set('navAnimation', loginAnimation);

@liamdebeasi
Copy link
Member

Hi everyone,

After discussing more with the team we have decided on two changes:

  1. The Config.set() method will not be removed in Ionic 5; however, it will be removed in Ionic 6.
  2. We will be providing a new deprecation warning in Ionic 4 along with links to updated docs on how to migrate your apps to a new solution.

For the use cases that were described here, the relevant Ionic components provide properties that allow you to customize different behaviors in your app (ex. ion-back-button has a text property, and ion-router-outlet has an animation property).

It is recommended that you update your applications to make use of these properties, rather than rely on the Config.set method. We intended Config to be something that is set once on app starts, and not something that is changed over the lifespan of the application. In other words, Config is not designed to be reactive.

I hope this clears things up. Please let me know if there are any questions. Thanks!

@JesperBalslev
Copy link

Thanks for the reply.

For the use cases that were described here, the relevant Ionic components provide properties that allow you to customize different behaviors in your app (ex. ion-back-button has a text property, and ion-router-outlet has an animation property).

Correct me if i am wrong, so whenever i have a back button:

<ion-back-button defaultHref="app/start"></ion-back-button>
<ion-back-button></ion-back-button>

i will now need to write it like this everywhere:

<ion-back-button [text]="{{ 'APP.backButton' | translate }}" defaultHref="app/start"></ion-back-button>
<ion-back-button [text]="{{ 'APP.backButton' | translate }}"></ion-back-button>

@liamdebeasi
Copy link
Member

liamdebeasi commented Nov 14, 2019

That's correct. It might make sense to create some kind of service to make that all easier to maintain. (I.e. if you decide to change APP.backButton to APP.BACKBUTTON you would only have to change it in the service rather than in every ion-back-button).

We realize this is a bit of a departure from how your apps are currently set up, which is why we decided to remove Config.set in Ionic 6 instead. Thanks!

@swbradsh
Copy link

For the use cases that were described here, the relevant Ionic components provide properties that allow you to customize different behaviors in your app (ex. ion-back-button has a text property, and ion-router-outlet has an animation property).

What is the recommended way for a child page component to get a reference to the ion-router-outlet in order to change the animation?

@liamdebeasi
Copy link
Member

You can do the following:

import { IonRouterOutlet } from '@ionic/angular';

constructor(private routerOutlet: IonRouterOutlet) {}

ngOnInit() {
  this.routerOutlet.nativeEl.animation = this.someAnimationFunction;
}

@JesperBalslev
Copy link

Thanks @liamdebeasi and the rest of the team. Keep up the good work.

@swbradsh
Copy link

this.routerOutlet.nativeEl.animation = this.someAnimationFunction;

@liamdebeasi I'm going back and refactoring my app to not use Config. I tried your example and it doesn't work.

Property 'nativeEl' is private and only accessible within class 'IonRouterOutlet'

I'm using @ionic/angular 4.11.2.

@liamdebeasi
Copy link
Member

Oddly enough I just ran into that issue myself. I plan on making nativeEl public in the next release of Ionic.

As a workaround for now you should be able to do:

(this.routerOutlet as any).nativeEl.animation = this.someAnimationFunction;

@KyDenZ
Copy link
Contributor

KyDenZ commented Dec 3, 2019

I also use config.set to put the IOS mode only on the web version:

if (this.platform.is('desktop')){
      this.config.set('mode', 'ios')
}

Is there an opportunity to do the same thing with Ionic4?

@liamdebeasi
Copy link
Member

liamdebeasi commented Dec 4, 2019

The future-proof way would be to set that config option in IonicModule.forRoot in app.module.ts. Unfortunately, at that point in time you will not have access to the Platform Service, so it is probably easiest to just copy the desktop platform detection code into your app directly: https://github.com/ionic-team/ionic/blob/master/core/src/utils/platform.ts#L89-L93.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants