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

[BUG] mgt-todo doesn't support initial-id and target-id #2266

Closed
sebastienlevert opened this issue May 9, 2023 · 6 comments · Fixed by #2407
Closed

[BUG] mgt-todo doesn't support initial-id and target-id #2266

sebastienlevert opened this issue May 9, 2023 · 6 comments · Fixed by #2407
Assignees
Labels
Milestone

Comments

@sebastienlevert
Copy link
Contributor

It's currently impossible to programmatically set the task list ID we'd love to see automatically load. Both MgtTaskBase properties (initialId and targetId) are never used within the component.

@sebastienlevert sebastienlevert added bug Something isn't working Needs: Triage 🔍 labels May 9, 2023
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Graph Toolkit May 9, 2023
@ghost
Copy link

ghost commented May 9, 2023

Hello sebastienlevert, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

@sebastienlevert sebastienlevert moved this from Needs Triage 🔍 to Todo 📃 in Graph Toolkit May 9, 2023
@sebastienlevert sebastienlevert added this to the v3.0.0 milestone May 9, 2023
@musale
Copy link
Contributor

musale commented May 30, 2023

Hello @sebastienlevert, I have done some digging into this issue and I have findings I would like to discuss.

mgt-todo uses mgt-picker to load the task lists

This is a blocker because it utilizes the /me/to/lists endpoint.

/**
* Render the generic picker.
*
*/
protected renderPicker() {
return mgtHtml`
<mgt-picker
resource="me/todo/lists"
scopes="tasks.read, tasks.readwrite"
key-name="displayName"
placeholder="Select a task list"
></mgt-picker>
`;
}

This makes it a challenge to use either target-id or initial-id as per their documentation. My understanding is target-id will only show a single task list on the dropdown. The initial-id will have the specified id as the first task on the dropdown list.

/**
* if set, the component will only show tasks from the target list
*
* @type {string}
*/
@property({ attribute: 'target-id', type: String })
public targetId: string;
/**
* if set, the component will first show tasks from this list
*
* @type {string}
* @memberof MgtTodo
*/
@property({ attribute: 'initial-id', type: String })
public initialId: string;

The task lists will always be loaded and displayed on the UI - even if we filter them out in the todo backend. Filtering in the todo backend is also impossible because mgt-picker is not accessible.

MgtTaskBase has an abstract renderPicker method

Another way to get access to the rendered task list would have been to manipulate the picker logic. However, this is done in the super class that is also used with mgt-tasks.

Recommendation

The documentation for the initial-id and target-id clearly show this is a bug. However, it cannot be fixed with the current updated state of the component.

One approach is to allow mgt-picker to take in a list of values to render. This way, we can load the required task lists and pass them to the mgt-picker for display manually:

<mgt-picker custom-list="[{id: 1, title: 'list item one'}]"></mgt-picker>

In our case, we filter out the task lists and remain with only one for target-id or set the initial-id as the first item on the list.

Another consideration should be de-coupling mgt-todo from MgtTaskBase. This should allow customization of behavior without affecting other components dependant on the super class.

What do you think? Cc @gavinbarron @Mnickii

@sebastienlevert
Copy link
Contributor Author

Could we ignore target-id in this case? I don't think this is a scenario that we necessarily want in this case. What would be ideal would be to select the "initial" list to be selected, but allow users to move to other lists.

Something we could potentially think for target-id could be disable the picker and select the initial list. That would remove the ability to the user to change their choice and might be easier to work with the current picker implementation.

Thoughts?

@Mnickii
Copy link
Collaborator

Mnickii commented May 31, 2023

Currently, mgt-picker just uses the response from mgt-get but the approach to allow mgt-picker to take in a list of values to render then load the required task lists and pass them to the mgt-picker for display manually could work.

As for decoupling mgt-todo from MgtTaskBase, I think this decision would depend on how the redesigned planner looks if we still plan to have all similar behavior in MgtTaskBase for both of them or have them decoupled.

@sebastienlevert another suggestion that could work is if we check for a target-id and if there's one, render tasks of the target-id, if there's none, render the current picker implementation?

@gavinbarron
Copy link
Member

So, for target-id, if it's provided then we don't need to render the picker at all, we could just have a label with the list name rendered. Sure we need to load that target list during the loadState method, but that seems reasonably straightforward. This looks similar to what @Mnickii is suggesting.

With initial-id we can use the selected-value attribute of the mgt-picker to set the selected value. This will need some testing to ensure that the selectionChanged event is emitted to correctly trigger the state changes, or some other manipulation of the data loading behavior.

@musale
Copy link
Contributor

musale commented Jun 6, 2023

Thank you all for your input on this. I have raised a PR that covers most of the suggestions raised, please review and advise on the PR.

@musale musale linked a pull request Jun 6, 2023 that will close this issue
6 tasks
@musale musale moved this from Todo 📃 to In Review 💭 in Graph Toolkit Jun 6, 2023
@github-project-automation github-project-automation bot moved this from In Review 💭 to Done ✔️ in Graph Toolkit Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants