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

Store the Response from the API as a File / Added reload button #22

Merged
merged 3 commits into from
May 8, 2023

Conversation

ronitjadhav
Copy link
Collaborator

#12

  • Store the response from the API as a file
  • Load from the file when opening the browser dialog
  • Add a button to reload resources

@ronitjadhav ronitjadhav requested a review from ismailsunni May 3, 2023 19:18
@ronitjadhav ronitjadhav linked an issue May 3, 2023 that may be closed by this pull request
3 tasks
@ismailsunni
Copy link
Collaborator

I think you haven't added the part where the browser will load from the file. Or did I miss something?

Could you also put a screenshot of the new UI/dialog, when you made UI changes? I usually use Flameshot to take a screenshot (or any tool is fine)

@ronitjadhav
Copy link
Collaborator Author

I think you haven't added the part where the browser will load from the file. Or did I miss something?

Could you also put a screenshot of the new UI/dialog, when you made UI changes? I usually use Flameshot to take a screenshot (or any tool is fine)

  1. The logic for the browser to load from the file is in the file api_client.py on line no: 17, 18, 19
  2. Below is the screenshot of the new UI

image

return response.json()


def reload_resources():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need this function, since it only has one statement. It's better to call get_all_resources with force_update set to true directly.

@@ -63,6 +65,15 @@ def populate_resources(self):
item = ResourceItem(resource)
self.resource_model.appendRow(item)

def reload_resources(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to merge this function with populate_resources and add a force_update parameter on the function (default to false). And connect the reloadPushButton with force_update to true

@ismailsunni
Copy link
Collaborator

@ronitjadhav could you also update the button? Put a label under the Resource Type and the button next to it. To make them look tidy. I think it's good to have a tool button instead a push button. You can use reload icon that is included on QGIS.

@ronitjadhav
Copy link
Collaborator Author

@ismailsunni
I have made changes mentioned by you:

  1. Removed reload_resources() function, merged the function with populate_resources.
  2. Updated the alignment of reload resources
  3. Replaced push button with tool button and added an icon

image

@ismailsunni ismailsunni self-requested a review May 8, 2023 06:29
Copy link
Collaborator

@ismailsunni ismailsunni left a comment

Choose a reason for hiding this comment

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

Thanks, Ronit, LGTM now.

There are some small tidy up that we can add later. I will create another ticket for it.

@ismailsunni ismailsunni merged commit 9cabdf9 into main May 8, 2023
@ronitjadhav ronitjadhav deleted the 12-store-the-response-from-the-api-as-a-file branch May 8, 2023 07:26
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.

Store the Response from the API as a File
2 participants