Progress Bubbles in header #16327
Progress Bubbles in header #16327
Conversation
| if (experiments.isEnabled('progressBubbles')) { | ||
| ExportedProgressBubble = NewProgressBubble; | ||
| } | ||
| export default ExportedProgressBubble; |
There was a problem hiding this comment.
I wanted to be able to be able to work on our new ProgressBubble class with minimal risk of regression existing bubbles. This is how I'm trying to accomplish that.
| // we plan to also get rid of this component in the near future, rather than | ||
| // clean it up more thoroughly (and risk regressions), I'm just making it | ||
| // explicit that courseOverviewPage is false here. | ||
| courseOverviewPage: false, |
There was a problem hiding this comment.
Potentially contentious/a little bit weird. I initially started making a cleanup of ProgressDot that accounted for the fact that courseOverviewPage is always false (since we finished the last redesign). However, there was a pretty fair amount of change, and I decided I was better off avoiding regressions and leaving this component messy, since it will be deleted after we're done with this next redesign.
| @@ -1,11 +1,15 @@ | |||
| // TODO: rename file now that it wraps ProgressBubble instead of dot | |||
There was a problem hiding this comment.
Addressing TODO before merge?
There was a problem hiding this comment.
I should probably make the TODO more clear, but my intent was to do this once this isnt behind an experiment any longer
| if (experiments.isEnabled('progressBubbles')) { | ||
| const onCurrent = currentLevelId && | ||
| ((level.ids && level.ids.map(id => id.toString()).indexOf(currentLevelId) !== -1) || | ||
| level.uid === currentLevelId); |
There was a problem hiding this comment.
What is level.ids vs level.uid? Why does currentLevelId match both, and why do we need to check both (i.e. is uid === currentLevelId sufficient)?
Also, I thought we had Array.prototype.includes polyfilled - it might be cleaner than indexOf here.
There was a problem hiding this comment.
I don't remember all the details offhand (I agree this is pretty hairy and might be improveable). FWIW, this was copied over from here https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/code-studio/components/progress/ProgressDot.jsx#L269
Some history:
Originally there was ProgressDot. It was used to represent progress everywhere.
Later we created ProgressBubble. This was our recent redesign. It is used on the script overview page and in the progress dropdown from the header. However, we still use ProgressDot to represent header progress.
We're doing another pass at redesigning progress bubbles (https://docs.google.com/document/d/11eQJ30FW48pFe7Eq7OGOr9fiQ5jj3EpJX79mrkcm5v4/edit#), and doing so in such a way that we should be able to have a single component for these - whether they're in the header or not.
This PR makes it so that (a) we use ProgressBubble everywhere instead of ProgressDot and (b) ProgressBubble (in the form of NewProgressBubble) can display small dots.
There is then a lot of follow on work to change to look of bubbles everywhere.