-
Notifications
You must be signed in to change notification settings - Fork 8
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
MWPW-163214: M@S Studio Splash Screen #105
base: main
Are you sure you want to change the base?
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
Commits
|
|
…eamline handleSelection logic
…Offer Selector Tool; enhance style with transition effect
*/ | ||
constructor(host, store) { | ||
this.store = store; | ||
this.value = store.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the initial value is get from store here and later at the line 31, the value in the store is not updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the controller I simply initialize the local state (this.value
). At line 31, the value in the story is already updated, updateValue
reacts to that, updating the local state, and asking for a rerender.
import MasSearch from './entities/search.js'; | ||
import MasFilters from './entities/filters.js'; | ||
|
||
class MasHashManager extends LitElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this need to be a web component?
why deeplink.js not sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, actually looking over it, it doesn't have to be a component. It can just be a function.
Deeplink does what store subscription does, so no point to have both and make things confusing.
In a simple function we can subscribe to the search
and filters
stores, and to the page hash (hashchange
), and make them update each other (no components needed for this).
I would prefer to do this separately, after merge if that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To update the state, we can either update hash, react to hash changes and update the store with these values
or update the store and the hash but not react to hash changes, after the first initialization.
both are possible with deeplink.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have something like:
And make sure they don't enter an infinite update loop.
Maybe we would even want to merge the search and filters stores, to be honest.
My point is, we don't really need the deeplink.js
anymore, which is supposed to be used in connectedCallback
and has a disposer (I mean ok, I understand we don't HAVE to use it in a component if we don't want to), when we can just call a function once that subscribes to stuff, and that's it. The state is outside the components now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree it could be a function, but yeah, could be done in next PR
} | ||
|
||
selecting = new StoreController(this, Store.selecting); | ||
selection = new StoreController(this, Store.selection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dupe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not, I'm subscribing to 2 different stores there 😄 One that specifies whether we're in selection mode, one that has the actual selection (an array of fragment ids)
To be honest, it might be better to set selection at fragment level (and I noticed on the main branch that there was something like that), but again, this is something we can come back to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that confusion is talkative: we should may be find another name that is less confusing for selecting
@@ -181,33 +302,271 @@ class MerchCardEditor extends LitElement { | |||
value="${this.fragment.tags.map((tag) => tag.id).join(',')}" | |||
@change=${this.#handeTagsChange} | |||
></aem-tag-picker-field> | |||
`; | |||
<p>Fragment details (not shown on the card)</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specific to merch cards but common to any fragment, hence it was put in editor-panel.js
.
Please move any form logic that is not specific to merch-card, back to editor-panel.js
|
||
get toolbar() { | ||
return html`<div id="editor-toolbar"> | ||
<sp-action-group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action buttons should not be in merch-card-editor, please bring back editor-panel. :-)
quiet | ||
> | ||
<sp-action-button | ||
label="Move left" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tested your change and thanks a lot for this and all the efforts! However i personally think we need another round of polish before getting it in. I understand you developed from quite a wild initial state and this is painful, but i do think your PR should be at a stage where we can - quickly - then iterate to something good. Typically i don't like current file structure, but i really don't consider blocking. I'll try to illustrate what i dislike and seems structural to me with following fictional scenario: let's imagine mas studio toolbar has no search, but already in another place stuff that do search. so related store exists.
As a UI developer, i should probably only know about some hook to central state manager about 'search' that i can update with my UI element, sending there keys & values. Ideally because we know it's lit, we could even have some lit hooks that takes fields name & directly update the stores.
here i have to add to declare a controller
search = new StoreController(this, Store.search);
that i use in
if (this.loading.value || !this.search.value.query) return nothing;
return html`<span id="search-results-label"
>Search results for "${this.search.value.query}"</span
>`;
}
that i don't use in
placeholder="Search"
@change="${extractValue(this.updateQuery)}"
@submit="${preventDefault(extractValue(this.updateQuery))}"
value=${this.search.value.query}
size="m"
></sp-search>
that is using
Store.search.update((prev) => ({ ...prev, query: value }));
}
and to me that's the problem. I guess i'm fine declaring a controller, but:
- i don't understand why we have "value" and "query" to handle. to me, there is a superflous level of structure here,
- i don't understand why i do have to declare a change event handler for something we do know how it works
here i would see something like
return html`<span id="search-results-label"
>Search results for "${this.search.value}"</span
>`;
}```
and just
placeholder="Search"
@change="${this.search.changeHandler}"
@submit="${preventDefault(extractValue(this.updateQuery))}"
value=${this.search.value.query}
size="m"
></sp-search>
having written it, i guess we could make those changes afterward. Will update to comment |
@yesil @Axelcureno here are feedbacks w.r.t ticket AC's and issues:
3)Welcome message - i believe should display user's name fetched from IMS profile - currnetly its hardcoded
|
Adds splash screen experience to M@S Studio.
Resolve MWPW-163214
Test URLs: