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 schema and response id to local storage #55

Merged
merged 7 commits into from
Sep 4, 2023

Conversation

petechd
Copy link
Contributor

@petechd petechd commented Aug 29, 2023

What is the context of this PR?

This adds js localStorage property usage to launcher. Schema name and response id are now saved and can be retrieved or deleted. New buttons added to the launcher html page and extra option of clearing local storage added, otherwise loading schema from url is permanently disabled when storage elements exist and have to be cleared manually.

How to review

Run locally using go run launch.go and test all the options manually.

@petechd
Copy link
Contributor Author

petechd commented Aug 29, 2023

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Working as intended - the saving is great 🎉
Only though I have re button, is it might be more intuitive for the odd other person using launcher if it is next to the field
maybe something like
Screenshot 2023-08-30 at 11 51 40
?
not sure what you think of something like that? but happy to go as is, either works so feel free to leave that one. If the button is at the bottom I think I'd also go with it saying "Load Previous Response ID" instead for clarity

@petechd
Copy link
Contributor Author

petechd commented Aug 31, 2023

Working as intended - the saving is great 🎉 Only though I have re button, is it might be more intuitive for the odd other person using launcher if it is next to the field maybe something like Screenshot 2023-08-30 at 11 51 40 ? not sure what you think of something like that? but happy to go as is, either works so feel free to leave that one. If the button is at the bottom I think I'd also go with it saying "Load Previous Response ID" instead for clarity

This probably looks better in-line with that field as on your image, looks great, I will try to add it this way.

@petechd
Copy link
Contributor Author

petechd commented Aug 31, 2023

Working as intended - the saving is great 🎉 Only though I have re button, is it might be more intuitive for the odd other person using launcher if it is next to the field maybe something like Screenshot 2023-08-30 at 11 51 40 ? not sure what you think of something like that? but happy to go as is, either works so feel free to leave that one. If the button is at the bottom I think I'd also go with it saying "Load Previous Response ID" instead for clarity

Added now, we could potentially add some svg icon next to the existing arrow but since we were told not to overthink it we could just keep the button. The only problem now is the arrow moved and slightly to the right.

@katie-gardner
Copy link
Contributor

Working as intended - the saving is great 🎉 Only though I have re button, is it might be more intuitive for the odd other person using launcher if it is next to the field maybe something like Screenshot 2023-08-30 at 11 51 40 ? not sure what you think of something like that? but happy to go as is, either works so feel free to leave that one. If the button is at the bottom I think I'd also go with it saying "Load Previous Response ID" instead for clarity

Added now, we could potentially add some svg icon next to the existing arrow but since we were told not to overthink it we could just keep the button. The only problem now is the arrow moved and slightly to the right.

Yeah I agree 👍 arrow looks slightly wonky but not that badly - I think it could be fixed with a tiny bit of padding (probably that 0.3em) on the button and field_container__img and tweaking field_container__span but yeah probably don't need to go overboard so it does the job for me

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Could possibly fix CSS if its simple but extra features working nicely 👍 happy as is

Copy link
Contributor

@MebinAbraham MebinAbraham left a comment

Choose a reason for hiding this comment

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

👍

@petechd petechd merged commit 0fba340 into main Sep 4, 2023
@petechd petechd deleted the save-schema-name-response-id branch September 4, 2023 08:28
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.

3 participants