feat(creature): new creature_text editor #2889

Instantium · 2024-04-18T07:11:02Z

Improvements:
link to broadcast_text

FrancescoBorzi · 2024-04-18T21:43:47Z

hi @Instantium thank you very much for contributing at Keira3!

I haven't looked carefully into this PR yet (I will do it later), but I immediately noticed you pushed huge changes to the package-lock.json. This is probably unintended and should be reverted.

I know what could have been the cause and how to easily revert it. Ping me if you need any assistance!

Instantium · 2024-04-19T14:54:30Z

Reverted the changes to package-lock.json.

libs/features/creature/src/creature-text/creature-text.component.ts

libs/features/creature/src/creature-text/creature-text.service.ts

FrancescoBorzi · 2024-04-24T12:01:09Z

apps/keira/src/assets/i18n/en.json

@@ -85,7 +85,8 @@
      "SKINNING_LOOT": "Skinning Loot",
      "SPAWN": "Spawn",
      "SPAWN_ADDON": "Spawn Addon",
-      "SMARTAI": "SmartAI"
+      "SMARTAI": "SmartAI",
+      "TEXT": "Text"


we should add this new key in all other languages, it's okay to put the text in English as temporary and it can be translated later (but they key should exist)

FrancescoBorzi · 2024-04-24T12:03:07Z

libs/features/creature/src/creature-text/creature-text.component.html

+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="GroupID">GroupID</label>
+              <input [formControlName]="'GroupID'" id="GroupID" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="ID">ID</label>
+              <input [formControlName]="'ID'" id="ID" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="Type">Type</label>
+              <input [formControlName]="'Type'" id="Type" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="Language">Language</label>
+              <input [formControlName]="'Language'" id="Language" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="Probability">Probability</label>
+              <input [formControlName]="'Probability'" id="Probability" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="Emote">Emote</label>
+              <input [formControlName]="'Emote'" id="Emote" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="Duration">Duration</label>
+              <input [formControlName]="'Duration'" id="Duration" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="Sound">Sound</label>
+              <input [formControlName]="'Sound'" id="Sound" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="BroadcastTextId">BroadcastTextId</label>
+              <input [formControlName]="'BroadcastTextId'" id="BroadcastTextId" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">
+              <label class="control-label" for="Type">TextRange</label>
+              <input [formControlName]="'TextRange'" id="TextRange" type="number" class="form-control form-control-sm" />
+            </div>


Bonus: add the tooltip to these fields to give a (short) explanation of what they do.

Example:

<i class="fas fa-info-circle ms-1" placement="auto" [tooltip]="'CREATURE.TEMPLATE.SPEED_SWIM' | translate"></i>

FrancescoBorzi · 2024-04-24T12:04:02Z

libs/features/creature/src/creature-text/creature-text.component.html

+              <label class="control-label" for="Type">TextRange</label>
+              <input [formControlName]="'TextRange'" id="TextRange" type="number" class="form-control form-control-sm" />
+            </div>
+            <div class="form-group col-12 col-sm-6 col-md-4 col-lg-2 col-xl-2">


I think we can make this field a bit bigger, so removing the col-xl-2 class should be enough

FrancescoBorzi · 2024-04-24T12:04:37Z

libs/features/creature/src/creature-text/creature-text.component.ts

+  changeDetection: ChangeDetectionStrategy.OnPush,
+  selector: 'keira-creature-text',
+  templateUrl: './creature-text.component.html',
+  styleUrls: ['./creature-text.component.scss'],


since this is empty, the file creature-text.component.scss can be deleted and this line removed

FrancescoBorzi · 2024-04-24T12:06:50Z

libs/features/creature/src/creature-text/creature-text.integration.spec.ts

+  let fixture: ComponentFixture<CreatureTextComponent>;
+  let queryService: MysqlQueryService;
+  let querySpy: Spy;
+  let handlerService: CreatureHandlerService;
+  let page: CreatureTextPage;


I'd prefer if we avoided shared-variables across tests and instantiate and return them in the setup function.

For example: https://github.com/azerothcore/Keira3/blob/master/libs/features/creature/src/creature-template-spell/creature-template-spell.integration.spec.ts#L33

FrancescoBorzi · 2024-04-24T12:07:59Z

libs/features/creature/src/creature-text/creature-text.integration.spec.ts

+  });
+
+  describe('Editing existing', () => {
+    beforeEach(() => setup(false));


this beforeEach will become obsolete with the new setup structure suggested in my comment above

FrancescoBorzi · 2024-04-24T12:08:26Z

libs/features/creature/src/creature-text/creature-text.integration.spec.ts

+  }
+
+  describe('Creating new', () => {
+    beforeEach(() => setup(true));


this beforeEach will become obsolete with the new setup structure suggested in my comment above

FrancescoBorzi · 2024-04-24T12:10:38Z

@Instantium very well done, I just left a bunch of minor comments and once they are addressed I think this is ready to go!

thank you very much for your contributions.

PS: I'm working on the page_text already, I will submit a PR for that later.

Instantium · 2024-04-24T16:32:51Z

I added the tooltips as suggested. It just made sense to also add the SingleValueSelector to TextType and TextRange.

Helias · 2024-04-24T17:01:57Z

Some errors in the pipeline:

  /home/runner/work/Keira3/Keira3/libs/features/creature/src/creature-text/creature-text.integration.spec.ts
     1:10  error  'ComponentFixture' is defined but never used. Allowed unused vars must match /^_/u  @typescript-eslint/no-unused-vars
    13:8   error  'Spy' is defined but never used. Allowed unused vars must match /^_/u               @typescript-eslint/no-unused-vars

FrancescoBorzi · 2024-04-26T12:58:12Z

apps/keira/src/assets/i18n/es.json

+      "EMOTE": "The emote that the creature plays when the text is executed. Value to use in this field can be obtained from the emote.dbc",
+      "DURATION": "Time in ms to see text. 0 is default and calculated by core.",
+      "SOUND": "The sound entry this creature will play at the same time the text is executed. Sounds are found in SoundEntries.dbc.",
+      "BROADCAST_TEXT_ID": "Id of the equivalent text found in broadcast_text"


maybe @pangolp can hep with the Spanish translations (in a separate PR of course, this should be merged meanwhile)

FrancescoBorzi · 2024-04-26T13:00:56Z

very well done @Instantium

thanks for your valuable contribution!

As I said, I will soon push the page_text editor myself.

Instantium force-pushed the feature/creature_text branch from 6f9174c to 6e7118f Compare April 18, 2024 07:51

Instantium force-pushed the feature/creature_text branch 2 times, most recently from ae7aed6 to 89b8f72 Compare April 19, 2024 14:53

Helias reviewed Apr 21, 2024

View reviewed changes

libs/features/creature/src/creature-text/creature-text.component.ts Outdated Show resolved Hide resolved

Helias reviewed Apr 21, 2024

View reviewed changes

libs/features/creature/src/creature-text/creature-text.service.ts Outdated Show resolved Hide resolved

Instantium force-pushed the feature/creature_text branch from 89b8f72 to 452e49b Compare April 23, 2024 10:59

Instantium requested a review from Helias April 23, 2024 11:08

FrancescoBorzi reviewed Apr 24, 2024

View reviewed changes

FrancescoBorzi changed the title ~~Feat: Added creature-text~~ feat(creature): new creature_text editor Apr 24, 2024

Instantium added 2 commits April 24, 2024 17:12

Added creature-text

3bdb704

Addressed review comments

61ebd6a

Instantium force-pushed the feature/creature_text branch from 452e49b to 61ebd6a Compare April 24, 2024 15:12

Instantium added 6 commits April 24, 2024 17:23

Added missing translation keys

3e3d36b

Removed empty scss file and reference

d0d2cff

Made comment field always two columns wide

b124a03

Added Tooltips for creature-text columns

e642dca

Added SingleValueSelector to TextType and TextRange

6514cbf

Removed shared-variables from tests

bde7b96

Instantium requested a review from FrancescoBorzi April 24, 2024 16:32

Fixed linting issues

4f3e9c3

Helias approved these changes Apr 25, 2024

View reviewed changes

FrancescoBorzi reviewed Apr 26, 2024

View reviewed changes

FrancescoBorzi approved these changes Apr 26, 2024

View reviewed changes

FrancescoBorzi merged commit ee953c1 into azerothcore:master Apr 26, 2024
12 checks passed

Uh oh!

feat(creature): new creature_text editor #2889

feat(creature): new creature_text editor #2889

Uh oh!

Conversation

Instantium commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrancescoBorzi commented Apr 18, 2024

Uh oh!

Instantium commented Apr 19, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrancescoBorzi commented Apr 24, 2024

Uh oh!

Instantium commented Apr 24, 2024

Uh oh!

Helias commented Apr 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FrancescoBorzi commented Apr 26, 2024

Uh oh!

Uh oh!

Instantium commented Apr 18, 2024 •

edited

Loading