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

ENDOC-507 Widget communication #575

Merged
merged 3 commits into from
Sep 23, 2022
Merged

ENDOC-507 Widget communication #575

merged 3 commits into from
Sep 23, 2022

Conversation

Lyd1aCla1r3
Copy link
Contributor

No description provided.

@Lyd1aCla1r3 Lyd1aCla1r3 marked this pull request as ready for review September 22, 2022 01:24
``` bash
npm start
```
## Modify the Publisher MFE

### Create Custom Event
Copy link
Collaborator

Choose a reason for hiding this comment

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

Create the Custom Event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the original didn't use articles in the sub headers

Copy link
Collaborator

Choose a reason for hiding this comment

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

noticed the headers below used them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i liked it better with the articles and started adding them in but then second-guessed it and reverted; i guess i missed a couple. changed my mind again tho, so back to using articles


Update `publisher-widget/src/index.js`.
2. To import the custom element, update `publisher-widget/src/index.js`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing you add the two import statements below but 'update' seems too vague.

And this forces the reader to spend more time comparing the doc to this snippet, instead of just copying it and inserting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. update means replace the contents. in other tutorials we've edited this language to make that explicit but i feel like the intent is understood so i left as-is. to expand upon it seems unnecessary because if you're familiar with the file content update=replace should be obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jyunmitch struggling with this one... this tutorial is identified as intermediate wrt our learning paths... i asked nathan to weigh in


``` js
import './index.css';
import './PublisherWidgetElement';
```

#### Test Custom Element
3. To test the MFE, update `publisher-widget/public/index.html` and view the app in the browser:
Copy link
Collaborator

Choose a reason for hiding this comment

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

would clarify what to do with body statement below. Again update is too vague and they would have to kind of guess if they did the right thing to add or replace.


Update `publisher-widget/src/App.js`.
1. Update `publisher-widget/src/App.js` to dispatch an event:
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here with regard to update, it can simply say add

#### Import Custom Element

Update `subscriber-widget/src/index.js`.
2. To import the custom element, update `subscriber-widget/src/index.js`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above, update is vague

...
</body>
```

### Display Custom Event

Update `subscriber-widget/src/App.js`.
1. Update `subscriber-widget/src/App.js` to display the event:
Copy link
Collaborator

Choose a reason for hiding this comment

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

again the same, are you supposed add the whole thing, bits of it, or what?

2. Open the `.env` file, and enter the `PUBLIC_URL` where the micro frontend will be hosted.

Example:
To add the publisher and subscriber MFEs to Entando, run the following commands from the root folder of each:
Copy link
Collaborator

Choose a reason for hiding this comment

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

each reads like it's referencing publish and subscriber, not the MFEs, even though indirectly they are mfes but I was thinking what publisher directory?


1. Go to `Pages` → `Management`
Set up the widgets on an existing page or [create your own page](../../compose/page-management.md). The following steps assume you'll use the `Home` page.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Set up >> place
thought it was referring to some configuration settings


6. In the `WIDGETS` sidebar on the right:
6. From the `Widgets` tab in the right sidebar, drag your publisher and subscriber widgets into `Frame 1` and `Frame 2`
Copy link
Collaborator

Choose a reason for hiding this comment

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

drag >> drag and drop
it doesn't matter which goes in which frame, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dragging the widget into the frame performs a drag and drop

it doesn't. the way we read from left to right, top to bottom would map to publisher on left/top but that's not worth mentioning

Copy link
Collaborator

Choose a reason for hiding this comment

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

not everyone reads left to right. I would totally read this one to mean put it in any frame

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, all the more reason to omit specific placement from the instruction. placement has no impact on functionality and is not subject to any constraints; the user is free to place the widgets in the layout of their choice. because the widgets exhibit some causality, the user will likely place them according to convention for cause and effect

@Lyd1aCla1r3
Copy link
Contributor Author

@nshaw what are your thoughts on adding some language to clarify that update means replace the contents of? there's one instance where update means change a snippet, which would require different clarifying language. the instructions were clear to me as-is, so i'd assume any dev following this tutorial wouldn't need it spelled out??

@nshaw
Copy link
Member

nshaw commented Sep 22, 2022

@Lyd1aCla1r3 I've definitely had people get confused on that one. It might be nice to use different language when you replace the whole file, versus adding/updating a section. It seems like this tutorial isn't quite as explicit as some tutorials in that sense.

@nshaw nshaw merged commit fce171e into main Sep 23, 2022
@nshaw nshaw deleted the ENDOC-507 branch September 23, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants