Skip to content

Conversation

@grayn90
Copy link
Contributor

@grayn90 grayn90 commented May 2, 2024

I was unable to build suggestions-input.element.ts until I made the following changes.

Description

  • Remove duplicate export default UmbMySuggestionsInputElement;
  • Update import modal context and modal open logic to use the UmbModalTokenDefaults in modal-token.d.ts as the args.

Type of suggestion

  • Typo/grammar fix
  • Updated outdated content
  • New content
  • Updates related to a new version
  • Other

Product & version (if relevant)

v14.0.0-rc2

Deadline (if relevant)

When should the content be published?

I was unable to build suggestions-input.element.ts until I made the following changes. 

- Remove duplicate export default UmbMySuggestionsInputElement; 
- Update import modal context and modal open logic to use the UmbModalTokenDefaults as the args.
@alina-tincas
Copy link
Contributor

Hi @grayn90 thank you for the PR 🙌

I have tried the whole file to check if it is working, however in my case, the code is still complaining about the .placeholder="${this.placeholder}"
image

I was wondering if you get the same on your side? 🙈

@grayn90
Copy link
Contributor Author

grayn90 commented May 2, 2024

Hi @alina-tincas, thanks for the quick review!

I don't get that error on my side, perhaps because I followed all parts 1-4.

Here is my umbraco-packahge.json but please let me know if you want me to send additional files.

umbraco-package.json

I will also start the tutorial from the beginning then follow the step 3 from the PR to see if I can replicate.

@alina-tincas
Copy link
Contributor

Hi @grayn90 I have tried this morning to make it work, and it seems that there are some missing pieces on this article:

  1. As I understand from the tutorial is that we work only on one .ts file which is suggestions-property-editor-ui.element.ts and with each article we build on top of it
  2. However this article seems to be outdated in name as well, as it uses a different element name and file as well. (instead of @CustomElement('my-suggestions-property-editor-ui') export default class MySuggestionsPropertyEditorUIElement it uses @CustomElement("my-suggestions-input") export default class UmbMySuggestionsInputElement and the file name is also different
  3. It seems that from the default "See the entire file" it is also missing some parts from the previous and next article such as:
    image

Can you please confirm if you have created a new file or used the same file from previous articles of the tutorial (https://docs.umbraco.com/umbraco-cms/v/14.latest-rc/tutorials/creating-a-property-editor/adding-configuration-to-a-property-editor) and built on top of this? 🤔

Update integrating-context-with-a-property-editor.md
@grayn90
Copy link
Contributor Author

grayn90 commented May 3, 2024

Hi @alina-tincas,

While reviewing yesterday I noticed the same thing and rewrote the page using the 'my-suggestions-property-editor-ui' file from the previous article as a base.

I have merged this update into my patch-1 branch for your review.

@alina-tincas
Copy link
Contributor

alina-tincas commented May 3, 2024

Hi @grayn90 thank you for the update 🙌

I have really been putting my mind to work here and still cannot make the Integrating context with a Property Editor updated code to work on my side. I have also checked the Integrating services with a Property Editor and this one seems to work as intended with your updated code from the other PR.

Checking those 2 articles side by side it seems that the Integrating services with a Property Editor article includes same steps from Integrating context with a Property Editor:
image

So in this case I believe it would be best to delete the Integrating context with a Property Editor article completely OR move these 2 content sections from Integrating services with a Property Editor to the Integrating context with a Property Editor:

image

What do you think about this? 💡

@grayn90
Copy link
Contributor Author

grayn90 commented May 3, 2024

Hi @alina-tincas, Sorry for taking so much of your time :).

My preference in this case would be to combine these as the tutorial is mostly working with contexts and the only mention of services is in the title of the 4th step. Even the summary does not mention integrating services:

image

If you agree please let me know if you want me to carry out the merge of Integrating services with a Property Editor and Integrating https://docs.github.com/github/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntaxcontext with a Property Editor

@alina-tincas
Copy link
Contributor

No worries @grayn90 we are both trying to achieve the same thing here, to have the docs up to date 🙌

Sounds really good with combining the sections and you are more than welcome to give it a try and combine the two of them. #h5YR

I would suggest using as a starting point the content from the "Integrating services with a Property Editor" article as I have seen that it includes more or less same steps as the ones from the "Integrating context with a Property Editor" (which are also more up to date with your other PR - full example) and remove one or the other articles 😊

Let me know if you need any further assistance while combining the articles👍

@grayn90
Copy link
Contributor Author

grayn90 commented May 3, 2024

Hi @alina-tincas I have updated once more so that Integrating context with a Property Editor is now a combination of step 3 and 4.

I have not deleted the "Integrating services with a Property Editor" yet on this branch.

Thanks again and have a great weekend.

@alina-tincas
Copy link
Contributor

alina-tincas commented May 6, 2024

Thank you @grayn90 🙌 I had a nice weekend, hope it was the same for you 😊

I have made some adjustments and will make a few more once am merging this PR in 👍

I will also remove the services article, and close your other PR: #6069

Thank you for your contribution #h5YR

@alina-tincas alina-tincas merged commit ec91ff5 into umbraco:main May 6, 2024
gitbook-com bot pushed a commit that referenced this pull request May 6, 2024
@grayn90
Copy link
Contributor Author

grayn90 commented May 7, 2024

Hi @alina-tincas,

I've taken a look as well and it all looks great.

Thanks again!

@alina-tincas
Copy link
Contributor

Thank you as well 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants