-
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
bug: milestone alignment - migrate rendering to d3 #237
Comments
Pausing this in favor of ipfs/ipfs-webui#2079 |
I pushed the code to the linked branch, here is the current status of the UI with d3: |
I should've done a better job explaining the decision to use HTML and CSS instead of D3 for this project. (I'll transfer decisions made in DMs and Google Docs to GitHub in the next few days for better clarity) Here's a bit of context behind the decision (my message from a conversation with @wilkyr31d regarding implementation details):
I still believe D3 isn't the right tool for the job here. It adds too much complexity and increases the maintenance cost for very little upside. FWIW, GitHub's new roadmap feature uses CSS flexbox with a structure similar to CSS grid and our implementation: https://github.com/orgs/nikas-org/projects/13/views/5. The grid layout also makes it easy to implement features like the list view (#120) using placement and alignment features: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Grid_Layout/Basic_Concepts_of_Grid_Layout Here's a screenshot with some quick modifications to the CSS for the list view (I'll add more details on #120): |
Done with MOST of the checklist.. except for the most difficult item: detailed view rendering. It shouldn't be too hard, but I wanted to flesh out a really nice simple view first. Check it out: starmap-d3-progress-2.mp4Note: Some roadmaps cause issues, and I will be looking into them. (updated the task list) |
I pushed up a PR #358 so users can try out the demo and give feedback.. You can try out the d3 rendering preview at https://starmaps-git-237-enhancement-bug-migrate-ren-3c536e-ipfs-ignite.vercel.app/roadmap/github.com/ipfs/ipfs-gui/issues/106 Things i’m looking for feedback on:
Most of these things are easy to change, but I'd rather get feedback from users early than late! |
Some feedback in slack:
|
Hey, just want to drop a note here and say that I feel totally ignored, and if the desire is for this project to be open source and collaborative, there needs to be better – well... – collaboration I raised some flags on changing the implementation to D3 both here (comment above) and in Slack, and while we had some more interactions in Slack, we didn't reach a conclusion on it. I'm not saying every decision has to go through the community or open-source contributors, but it'd be nice to have a comment stating the decisions you (or the owner/maintainer of the project, which isn't clear) make, especially when there was prior discussion around a change. I know it's way easier to get things moving without external collaboration, but if that's the current status, I think it needs to be stated somewhere so open-source contributors know when to jump in and help develop the project instead of wasting time and bothering maintainers. Again, just dropping this comment to collaborate with the project and its future so we can all see it grow as a healthy, open source project that receives contributions from everyone. |
@AlexxNica, I'm sorry to hear that you feel ignored. I understand how frustrating that can be. I've been posting updates to this issue to be open and collaborative but I missed an opportunity to announce that I was "officially" starting this work. This issue was never closed, which means the primary maintainers (IPFS Ignite - IPFS GUI & Tools team) felt that this issue was still in the realm of things to be done. It was recently decided that this work needs to be prioritized, so it was decided (without announcing; apologies) that I would start work on this last week. I am writing a separate document explaining why we're doing this and will share it in this issue when done. In short, the maintenance of the existing code could be better for the current maintainers, while achieving a much more accurate visual representation for the users. I understand that you feel differently, but the fact is that the two engineers currently responsible for this code feel this way. I understand that you may have some feedback as a user, and I definitely want to hear your thoughts. However, I do want to clarify that the implementation itself is not up for debate. Open-source collaboration does not mean we need to agree. We encourage you to provide your feedback on the open PR (#358) so that we can make Starmap better for everyone. We expect to have this work completed within the next week, and your input is valuable and will help us create a better product. |
@SgtPooki thanks for clarifying! 🙌 |
Cool! I took a quick look at the demo. A couple of comments:
|
This was previously changed to be more "human readable," but I can switch to YYYY-MM-DD format.
I have heard from some users that leaving the order provided in the issue is desired, but after looking at the roadmap a lot in the past week, I personally would prefer sorting in a way more like what you describe; Ideally, this would be configurable by the user. By preferring the provided children order (the order provided in the github issue) users should be able to control the order, without us having to maintain any complicated settings/configuration logic. This gives users the most control and us the least amount of work required to support that control. Also, if we were to order things by dates so that the leftest-X valued milestones are always more "top and left" than comparable milestones, I imagine we would frequently end up with a stair-stepping, non-collapsible view. If users want that view, they should be able to get it with the current logic.
Compressing the items into less vertical space was an explicit request from Molly during IPFS Thing: "I need to compress the items more; they take up too much vertical space." I've heard similar from other users and have frequently felt the same.
I believe it would complicate things much more significantly. The bin-packing logic is done and should be ready to use in more complicated roadmaps. (still to be confirmed and user-tested with Molly). If we add rows, we need to decide on what semantic meaning that actually has, and how users can specify that meaning with the current configuration options available to them (github issue markdown only) In a Starmap roadmap, I don't think we really have a concept of rows currently, other than in the detailed view, which is not necessarily a row but a grouping of multiple simple views for all the children of the root issue being viewed. I could see a future where we do consider the concept of rows/tracks via labels or markdown key/val or something, but that currently doesn't exist in Starmap that I'm aware of. Maybe we can describe more in the user guide about how the only relationship between items is the ETA and how they relate to other ETAs.
I believe mollys specific request was to have a "screenshot mode" button that would compress items, so maybe I need to just add a toggle that increases/decreases ySpacing in the binPacking function. This is a very easy logic change, but where does the knob go without getting in the way? (related: #166) |
ISO format is good because there is no ambiguity across international lines. If you are wanting to do a more english format, then I think "Jan 23, 2023" rather than "23 Jan, 2023" (I'm not familiar where that format is common, and it's certainly not the norm in North America).
I agree that issue-specified ordering is the best choice if not making this configurable as at least then the user is in control.
I wasn't suggesting to make "rows" a first class primitive. I agree that is more work. I was more thinking that if the children tasks are X, Y, Z then X, Y, and Z are on the own row (no bin-packing). (This is analogous to Github and Notion's timeline view).
To be scrappy, maybe just make it a query param for now and document it in the readme as an experimental feature? (It'll need to be a query param anyway in the future so that URLs are bookmarkable). |
Feedback notes: The default zoom level is unexpected, I would prefer to get ~2 quarters of time? Zooming and navigating (using a trackpad) works really well! When you zoom out far and have many overlapping items, it can be necessary to scroll vertically. That doesn't work for me when my pointer is over the roadmap, I have to move it to the side of the page to scroll. |
@AlexxNica, since there is no concept of time (, or start date for that matter), I would like to imagine, something due on Dec 1, comes due at 12am Dec 1. I think there is a difference in expectations. The calculations can be trivial to fix, if we can fix the expectations. IMHO this is way better in aligning milestones at the right position, which was not the case with CSS grid, I remember spending a lot of time in one of the older PRs to make issues align right with the grid. |
Thanks a ton for the helpful feedback @juliangruber!
Definitely. I thought of this but dropped the ball. I added an action item. I've been thinking of making the zoom level share-able (url param) to help with debugging and collaboration as well. Ideally, any "view" should be re-renderable by just sharing the URL
Yay! There's still some polish on zoom/pan sync that could be done but I'm glad it's at a good space for you, i spent a day last week polishing this up
I have been debating how to handle this. I think we would want the header to stay locked in place, and allow users to scroll vertically within the view. There's also the problem of very tall views and the need to screenshot the entirety of the roadmap, so we want to be able to allow scrolling, while also allowing users to see the entire roadmap if they want. I'll add an action item to address this. |
Definitely like this as a tenet to follow. (Obviously the thing we can control is someone's window size and user-agent, but this will still be great.) |
Certainly a minor issue that I don't think we need to prioritize, but I think a good rule of thumb is that the time we use is the very end of a given day. The reason I suggest that is because folks will naturally say, "this will be done by end of quarter 2023-06-30". In that case we should fill up through June. But again, not critical here. |
@whizzzkid Makes sense. (though I agree with @BigLep - I believe the most common understanding of due dates/ETAs refers to the end of that specific day - or within business hours in case of a product release, etc.) Clarifying that last comment: it wasn't meant to suggest using css grids, it was just some feedback in case that was a bug 😄 |
The latest changes update all milestone card ETAs to be "endof('day')" |
now with default view finding: Try out Now with linkable zoomconfig: Note: If you are going to pan, use either drag or horizontal swipe (trackpad). Both will end up causing jitter for some reason. I looked into it a lot today but spent too much time on it. I'm going to disable horizontal scrolling for panning if anyone has issues with the current functionality. |
@SgtPooki looks like the fetch issues for stale data is still problematic. Cleaning the storage makes it go away. Screen.Recording.2023-05-10.at.1.16.52.AM.mov |
it appears like #363 fixed the fetch errors. I bet it was the cachedResponse=undefined that was causing the failures. |
FYI: #358 has approval, and will be merged this week unless there are pressing concerns users have regarding the demo you can visit at https://starmaps-git-237-enhancement-bug-migrate-ren-3c536e-ipfs-ignite.vercel.app/ |
Milestone rendering is inaccurate and difficult to debug and get accurate because of the use of css-grids.
While this issue is intended to track the migration to something more accurate, here are some sub-items that have popped up due to the use of css-grid:
Starmaps still impacted by inaccurate css-grid rendering:
Tasks
The text was updated successfully, but these errors were encountered: