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

save map dropdown #1357

Merged
merged 5 commits into from
Feb 20, 2023
Merged

save map dropdown #1357

merged 5 commits into from
Feb 20, 2023

Conversation

7malikk
Copy link
Collaborator

@7malikk 7malikk commented Feb 15, 2023

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt test
  • code is in a uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updates
  • @mention the original creator of the issue in a comment below for help or for a review

UI Changes

saveMaps

@7malikk
Copy link
Collaborator Author

7malikk commented Feb 15, 2023

@jywarren here's the dropdown button for the saveMaps

<button id="downloadJSON" type="button" class="btn btn-sm dropdown-item" aria-label="Save map" >
Download Map
</button></li>
<li title="Save map in browser in JSON format">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we added one more option, which uses alert() to provide a bit more text about the limitations and uses of each option? What might we say to let people understand the difference?

Copy link
Collaborator Author

@7malikk 7malikk Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that sounds reasonable, something like an info icon?

I believe we could give a write-up like:
Saving your map in the browser means you can continue knitting when you return to mapknitter.org

I believe the share modal provides succinct info on what the downloaded JSON file could be used for.

@jywarren

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great but instead of mapknitter.org, let's say "this page in your current browser" to really get specific about what's required to recover the state. Sound good?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

Maybe we'd worry about recovering the state in another PR, but what would that flow be like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, one option could be to have this save in localstorage button open the history modal, and do it all in one PR, or do the history modal first, so there's something to open when we implement this button? That modal could have space to better explain things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it sounds strange that the second item in the Save button would then open the History modal, when there's another button for that. But consider this flow:

image

I think it makes sense to have the two forms of saving be together. But I also think that having the Saved Maps modal be the place where you can save or recover "in browser" maps makes sense. I can see how it's a little strange that Download immediately does something while Save in browser takes you somewhere else. But it also links saving and recovering together nicely.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not a bad idea, a tad confusing at the first glance but not bad.
I'll begin work on it, should it be done in this PR or another?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to do here, if you like? After all, it also makes sense that we should introduce the ability to recover a map before we introduce the ability to save one, otherwise people may save maps but not be able to recover them, you know?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think of an improved way to do it, I'm happy to discuss! Thanks Malik!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to do here, if you like? After all, it also makes sense that we should introduce the ability to recover a map before we introduce the ability to save one, otherwise people may save maps but not be able to recover them, you know?

Yea I totally agree
I'll get it on here

If you think of an improved way to do it, I'm happy to discuss! Thanks Malik!

Definitely
Thank you @jywarren

@7malikk
Copy link
Collaborator Author

7malikk commented Feb 17, 2023

Hello @jywarren
This is coming out quite nicely, here's a GIF:
saveMapModal

I'm moving to the aspect where the user clicks on recover to load the saved map
I stored each SavedMap's index as a dataset in the recover button, I intend to use this to filter out the particular map to load.

I may need to extract the function to accomplish this

What are your thoughts?

@jywarren
Copy link
Member

jywarren commented Feb 17, 2023 via email

@7malikk
Copy link
Collaborator Author

7malikk commented Feb 17, 2023

Wow it looks great!! Which function are you referring to? Sorry, I don't
quite follow.

I was referring to the reconstructMapFromJson here on line 260.

I was thinking I'd need to extract just a piece of it, but for reusability I could refactor it, to accept an Objectas a source to reconstruct the map.

@jywarren
Copy link
Member

Ah I see. Yes I think making it a more general purpose function that accepts an object makes sense!

@7malikk
Copy link
Collaborator Author

7malikk commented Feb 20, 2023

Hello @jywarren
Here's the final look of the modal:

recoverMap

What do you think?

@jywarren
Copy link
Member

jywarren commented Feb 20, 2023 via email

@7malikk
Copy link
Collaborator Author

7malikk commented Feb 20, 2023

@jywarren Yes it is ready for review and merge, Thank you!

const reconstructMapFromJson = async (jsonDownloadURL = false, savedMapObj) => {
if (jsonDownloadURL) {
const imageCollectionObj = await map.imgGroup.recreateImagesFromJsonUrl(jsonDownloadURL);
console.log(imageCollectionObj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I see a log here, mind removing it? Thanks!

// share map modal
const shareModal = document.getElementById('shareModal')
const modality = new bootstrap.Modal(shareModal)
shareMapBtn.addEventListener('click', () => {
bootstrap.Modal.getInstance(shareModal).show()
});

// history map modal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some complex but relatively self contained code which would be great to move into a module in a later PR!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts exactly @jywarren

@7malikk
Copy link
Collaborator Author

7malikk commented Feb 20, 2023

@jywarren I've removed all logs

@jywarren jywarren merged commit b86fd98 into publiclab:main Feb 20, 2023
@jywarren
Copy link
Member

Great job!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants