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: milestone alignment - migrate rendering to d3 #237

Closed
25 tasks done
SgtPooki opened this issue Dec 5, 2022 · 30 comments · Fixed by #358
Closed
25 tasks done

bug: milestone alignment - migrate rendering to d3 #237

SgtPooki opened this issue Dec 5, 2022 · 30 comments · Fixed by #358
Labels
bug Something isn't working effort/weeks enhancement New feature or request epic P0 post-MVP status/ready This issue has enough information to be worked on and no technical blockers or pre-reqs

Comments

@SgtPooki
Copy link
Contributor

SgtPooki commented Dec 5, 2022

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

Preview Give feedback
  1. SgtPooki
  2. SgtPooki
  3. SgtPooki
@SgtPooki SgtPooki added this to Starmap Dec 5, 2022
@SgtPooki SgtPooki moved this to Todo in Starmap Dec 5, 2022
@SgtPooki SgtPooki added enhancement New feature or request effort/weeks post-MVP status/ready This issue has enough information to be worked on and no technical blockers or pre-reqs P2 labels Dec 5, 2022
@SgtPooki SgtPooki moved this from Todo to Needs Prep Work in Starmap Dec 5, 2022
@SgtPooki SgtPooki added the epic label Dec 8, 2022
@SgtPooki SgtPooki moved this from Needs Prep Work to Todo in Starmap Jan 23, 2023
@SgtPooki SgtPooki added bug Something isn't working P0 and removed P2 labels Jan 24, 2023
@SgtPooki SgtPooki moved this from Todo to In Progress in Starmap Jan 31, 2023
@SgtPooki SgtPooki self-assigned this Jan 31, 2023
@SgtPooki
Copy link
Contributor Author

Getting the header positioning down:

image

@SgtPooki
Copy link
Contributor Author

dialing in on new mocks for grid header items:

image

@SgtPooki SgtPooki moved this from In Progress to Todo in Starmap Feb 1, 2023
@SgtPooki SgtPooki removed their assignment Feb 1, 2023
@SgtPooki
Copy link
Contributor Author

SgtPooki commented Feb 1, 2023

Pausing this in favor of ipfs/ipfs-webui#2079

@SgtPooki
Copy link
Contributor Author

SgtPooki commented Feb 1, 2023

I pushed the code to the linked branch, here is the current status of the UI with d3:

image

@AlexxNica
Copy link
Collaborator

AlexxNica commented Feb 10, 2023

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 didn't find any libraries to use for the renderer that wasn't overly complex, so I had to build one from scratch.

Almost all solutions for timeline-like things are using D3 and SVG/vector directly. The not-so-great thing about using D3/SVG/vector directly is that it doesn't have the usability/flexibility of HTML. Those libraries are great if all we're going to do is render a timeline and forget about it. But since we require a lot of interactivity from the user (rendering, clicking, event-related states), HTML and CSS would the best solution as far as I'm aware without the increased complexity of messing with vectors and path/positioning calculations for every element–which I don't have enough experience to make happen in a short time for an MVP.

This is specific to timelines, though. For rendering complex graph-like, connected things, there are great libraries out there. Since we need a timeline, those libraries don't fit our use case.

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):
image

@SgtPooki SgtPooki changed the title enhancement / bug: migrate rendering to d3 bug: milestone alignment - migrate rendering to d3 Feb 10, 2023
@SgtPooki
Copy link
Contributor Author

progress bars for d3 renderer are in

image

@SgtPooki
Copy link
Contributor Author

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.mp4

Note: Some roadmaps cause issues, and I will be looking into them. (updated the task list)

@SgtPooki SgtPooki linked a pull request Apr 28, 2023 that will close this issue
@SgtPooki
Copy link
Contributor Author

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:

  • require CMD/CTRL key for scrolling/panning methods?
  • swap pan/zoom directions? what is your intuition telling you?
  • header tick/date label transitioning on zoom, too soon? too late? customize-able?
  • text sizes, anything else visual…?
  • should we have more loose/tight zoom/pan limits?

Most of these things are easy to change, but I'd rather get feedback from users early than late!

@SgtPooki
Copy link
Contributor Author

Some feedback in slack:

  • It's unexpected that horizontal scroll does zooming
  • Zooming using my track pad's zoom gesture works really well
  • Therefore I'm wondering, why not use scroll for scrolling and zoom for zooming, for those who use a track pad? And offer alternatives for pure mouse users (+/- buttons or a slider)?
  • Zooming using my track pad's scroll gesture feels jittery
  • Header works well during zooming, I don't see why you'd need to make it customizable
  • Everything else looks great!

@AlexxNica
Copy link
Collaborator

AlexxNica commented Apr 29, 2023

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.

@SgtPooki
Copy link
Contributor Author

SgtPooki commented May 1, 2023

@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.

@AlexxNica
Copy link
Collaborator

@SgtPooki thanks for clarifying! 🙌

@BigLep
Copy link

BigLep commented May 2, 2023

Cool! I took a quick look at the demo. A couple of comments:

  1. We are using a date format like "23-Jan-2023". Can we do ISO8601 as we require in the issues themselves (e.g., 2023-01-23)?
  2. When I zoom out (see below), items don't order I would expect. I think we want to sort by endDate ASC, startDate ASC, childOrder ASC. Basically, whatever is completing first should be up first. Where there is a tie, look at start date, and where there is still a tie, use the ordering of in the children list.
    • Maybe even better here is to let this be choosable by the user, where they can decide whether to order by startDate, endDate, or children list order.

image

  1. Stepping back, I'm wondering whether as a simplifier, do we even need to binpack? What if we do one milestone per row? Does that make things simpler? I can see how binpacking makes things more compact on the screen, but it then more me it calls into question, "Why are two items on the same row? Do they have a certain relationship?" One item per row removes ambiguity.
    • Here again maybe having a configuration knob could be useful for turning on bin packing or not and we see which option people reach for.

@SgtPooki
Copy link
Contributor Author

SgtPooki commented May 2, 2023

We are using a date format like "23-Jan-2023". Can we do ISO8601 as we require in the issues themselves (e.g., 2023-01-23)?

This was previously changed to be more "human readable," but I can switch to YYYY-MM-DD format.

When I zoom out (see below), items don't order I would expect. I think we want to sort by endDate ASC, startDate ASC, childOrder ASC. Basically, whatever is completing first should be up first. Where there is a tie, look at start date, and where there is still a tie, use the ordering of in the children list.

  • Maybe even better here is to let this be choosable by the user, where they can decide whether to order by startDate, endDate, or children list order.

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.

Stepping back, I'm wondering whether as a simplifier, do we even need to binpack?

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.

What if we do one milestone per row? Does that make things simpler?

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.

Here again maybe having a configuration knob could be useful for turning on bin packing or not and we see which option people reach for.

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)

@BigLep
Copy link

BigLep commented May 3, 2023

This was previously changed to be more "human readable," but I can switch to YYYY-MM-DD format.

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).

Ordering

I agree that issue-specified ordering is the best choice if not making this configurable as at least then the user is in control.

What if we do one milestone per row? Does that make things simpler?

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).

This is a very easy logic change, but where does the knob go without getting in the way?

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).

@juliangruber
Copy link
Contributor

Feedback notes:

Screenshot 2023-05-05 at 09 16 50

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
Copy link
Collaborator

I noticed that the alignment might be a bit off. From the screenshot below, the ETA is set for December 1st but the card isn't in the December box:
Screenshot 2023-05-05 at 4 46 46 AM

@whizzzkid
Copy link
Collaborator

@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.

@SgtPooki
Copy link
Contributor Author

SgtPooki commented May 5, 2023

Thanks a ton for the helpful feedback @juliangruber!

The default zoom level is unexpected, I would prefer to get ~2 quarters of time?

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

Zooming and navigating (using a trackpad) works really well!

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

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.

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.

@BigLep
Copy link

BigLep commented May 5, 2023

Ideally, any "view" should be re-renderable by just sharing the URL

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.)

@BigLep
Copy link

BigLep commented May 5, 2023

From the screenshot below, the ETA is set for December 1st but the card isn't in the December box

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.

@AlexxNica
Copy link
Collaborator

@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.

@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 😄

@SgtPooki
Copy link
Contributor Author

The latest changes update all milestone card ETAs to be "endof('day')"

@SgtPooki
Copy link
Contributor Author

now with default view finding: Try out

https://starmaps-git-237-enhancement-bug-migrate-ren-3c536e-ipfs-ignite.vercel.app/roadmap/github.com/ipfs/ipfs-gui/issues/106

Now with linkable zoomconfig:

Check out https://starmaps-git-237-enhancement-bug-migrate-ren-3c536e-ipfs-ignite.vercel.app/roadmap/github.com/ipfs/ipfs-gui/issues/106#d3x=799.10&d3y=198.90&d3k=0.20

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.

@whizzzkid
Copy link
Collaborator

@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

@SgtPooki
Copy link
Contributor Author

@SgtPooki looks like the fetch issues for stale data is still problematic. Cleaning the storage makes it go away.

I'm hoping #363 will help us dive into those more

@SgtPooki
Copy link
Contributor Author

it appears like #363 fixed the fetch errors. I bet it was the cachedResponse=undefined that was causing the failures.

@SgtPooki
Copy link
Contributor Author

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/

@github-project-automation github-project-automation bot moved this from Triage 🩺 to Done ✅ in PLN Planning Tools - High Level Tracker May 19, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Starmap May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working effort/weeks enhancement New feature or request epic P0 post-MVP status/ready This issue has enough information to be worked on and no technical blockers or pre-reqs
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants