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

Feature request: Keeping spaces #62

Open
stevengunneweg opened this issue Jul 19, 2024 · 4 comments · Fixed by #63
Open

Feature request: Keeping spaces #62

stevengunneweg opened this issue Jul 19, 2024 · 4 comments · Fixed by #63
Assignees
Labels
enhancement New feature or request

Comments

@stevengunneweg
Copy link
Contributor

stevengunneweg commented Jul 19, 2024

I use this library for a while already and it works great but I need one additional feature. For this reason I created a fork but it feels ridiculous to maintain a fork for 1 tiny change.

The issue:

When I have a text that I want to cut to apply some additional HTML for example, the spaces are trimmed even though this is not desired.

E.g.
for the following HTML

<div>
  {{ 'translation' | translate | translateCut: 0 }}
  <a href="#">{{ 'translation' | translate | translateCut: 1 }}</a>
  {{ 'translation' | translate | translateCut: 2 }}  
</div>

"translation": "This is a |cut| sentence.",
will work fine since my HTML is responsible for placing the correct spaces.

However when my cut is at the end of a sentence like this
"translation": "This is a sentence that is |cut|.",
the HTML does not know whether a space is needed between the cuts so there will be either spaces on both sides of cut or none at all, neither is correct.

My fix

To fix this I made the following change to /projects/ngx-translate-cut/src/lib/ngx-translate-cut.pipe.ts (which could be optimised further)

-    const result = phrase ? phrase.trim() : '';
+    const result = phrase ? phrase : '';

This will keep the existing spaces from the original translation and just needs small adjustment to my html to work without any issues

<div>
  {{ 'translation' | translate | translateCut: 0 }}<a
    href="#">{{ 'translation' | translate | translateCut: 1 }}</a
  >{{ 'translation' | translate | translateCut: 2 }}  
</div>

If this is too much of a risk to just apply without it breaking for other users of this library it could become a configuration option in the forRoot implementation.
Personally I doubt that this change will break anything for other users since they are already responsible for the correct spaces in their HTML but it would help me a lot. Please consider my request.

@bartholomej bartholomej mentioned this issue Jul 21, 2024
@bartholomej bartholomej self-assigned this Jul 21, 2024
@bartholomej bartholomej added the enhancement New feature or request label Jul 21, 2024
@bartholomej
Copy link
Owner

Hey @stevengunneweg,
thank you for the feature request...

This change breaks all the test and trim() there was intended on purpose.

However, there is nothing stopping it from being done as an option that may come in handy for someone ;)

Try this new unreleased version: 18.1.0-next.1

npm install ngx-translate-cut@next

Set this option:

  import { NgxTranslateCutModule } from 'ngx-translate-cut';

  @NgModule({
   // ...
   imports: [
     // ...
     NgxTranslateCutModule.forRoot({
      trim: false
    }),
   ]
  })

...and let me know ;)

@stevengunneweg
Copy link
Contributor Author

@bartholomej thanks for your quick response and work on this request!

I'm using Angular standalone components which somehow doesn't seem to work well with the forRoot configuration of this library (I have not tried a non-standalone component to know whether it works in that setup).
When I import the NgxTranslateCutModule with the forRoot method in my app config as you described and I check all the properties in my standalone component (not allowed to call forRoot here) I see that the pipe doesn't load the options correctly.

app.config.ts

NgxTranslateCutModule.forRoot({
	separator: '*',
	trim: false,
}),

app.component.ts

@Component({
	standalone: true,
	imports: [CommonModule, RouterModule, TranslateModule, NgxTranslateCutModule],
	providers: [NgxTranslateCutPipe],
})
export class AppComponent {
	constructor(
		@Optional() @Inject(FOR_ROOT_OPTIONS_TOKEN) options: any,
		optionspipe: NgxTranslateCutPipe,
	) {
		console.log('!!!', options);
		console.log('!!!', optionspipe['options']);
		console.log(
			'!!! before:',
			JSON.stringify(optionspipe.transform('asdfas | other * label', 0)),
		);
		optionspipe['options']['trim'] = false;
		console.log(
			'!!! after:',
			JSON.stringify(optionspipe.transform('asdfas | other * label', 0)),
		);
	}
}

results in the following

> !!! {separator: '*', trim: false}
> !!! NgxTranslateCutOptionsService {separator: '|', trim: true} // <-- should be same settings as above
> !!! before: "asdfas"
> !!! after: "asdfas "

I thought that adding the providedIn property to the Injectable of the service might fix the issue but in my fork this has no effect on the situation. As of yet I have no clues how to resolve this.


I also suspect that the trim option check needs a small adjustment since a false value won't pass the condition. This however I can not verify since passing the property does not work for my project

- if (options.trim) {
+ if (options.trim !== undefined) {
	ngxTranslateCutOptionsService.trim = options.trim;
}

@stevengunneweg
Copy link
Contributor Author

stevengunneweg commented Jul 26, 2024

Update

I made several changes to my fork and everything seems to work now!

I've opened a pull request #71 where I implemented below changes and added some tests.

Service injectable options

I added providedIn: 'root', to the NgxTranslateCutOptionsService injectable options.

Standalone pipe

I made the NgxTranslateCutPipe standalone which seems to resolve the issues. This required me to change the module options to the following

@NgModule({
  imports: [NgxTranslateCutPipe],
  exports: [NgxTranslateCutPipe],
})

Undefined check for boolean option

I added the earlier suggestion to add an undefined check to ngxTranslateCutOptionsFactory. This was necessary since otherwise the property would not be set correctly.

@bartholomej
Copy link
Owner

bartholomej commented Aug 20, 2024

Great job Steven! And thank you!

Just try v18.1.0 and let me know ;)

@bartholomej bartholomej reopened this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants