-
Notifications
You must be signed in to change notification settings - Fork 25
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
Kuwait Theme: Map View Styling #2669
base: master
Are you sure you want to change the base?
Conversation
…pp-builder into feature-kw-map-view
…pp-builder into feature-kw-map-view
…pp-builder into feature-kw-map-view
…pp-builder into feature-kw-map-view
For your review @jfmcquade @esmeetewinkel |
…pp-builder into feature-kw-map-view
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 new path style looks nice. However, it doesn't reflect the design and so won't line up with the background image in the same way. I think that will have to be addressed in the future as a follow-up, appreciate this could be quite difficult to implement.
I've tidied and clarified some of the logic with b7263f4.
Additionally, I think we'll need to handle the "locked" status differently. @esmeetewinkel – currently, this PR sets the "locked" status through the same logic that determines the completion status of the task group: namely, if the task group is "not started" (i.e. none of its constituent tasks are completed), then the task group is considered "locked". However, this doesn't allow us to handle unlocking task groups. If the task group is not clickable when it is locked, then there I don't believe there is a way for the user to begin that task group. If the task group is clickable when locked, then the "locked" status is misleading. So I think we need to handle the "locked" status via a dedicated parameter, and handle the logic elsewhere (e.g. in a dedicated locked
column that gets updated through template logic). Does that sound right to you @esmeetewinkel? If so, I can update this PR. I think if we need to rush something in the short-term, we could leave the code as-is which would mean that a locked task-group could still be clicked on to enter that module.
Yes that sounds right |
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.
OK @esmeetewinkel, I've updated to expose an explicit locked
param. See final example in updated comp_progress_path template
PR Checklist
Description
Author Notes
wavy
variant accommodates. A new variant,curved
, is added in this update to support curvier paths.locked_image_asset
, to accommodate the locked icon that appears on top of the module.Attributes added to sheet:
variant
: "curved"locked_image_asset
Git Issues
Closes #2667
Screenshots
To visualise the map path, the color stroke here is dark
Otherwise the map path is a light color to contrast with the proposed background image
A
completed
module is highlighted with a blue border while anin_progress
module is highlighted with a yellow border