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

Present options to create an article for manual entry or return to the new entry dialog box when failed to find DOI ID ... #7870 #8203

Merged
merged 8 commits into from
Nov 8, 2021

Conversation

mrcstan
Copy link
Contributor

@mrcstan mrcstan commented Nov 1, 2021

Fixes stuck-in-library issue when the fetcher DOI failed to find an entry for an ID #7870. User can now choose to add entry manually instead of returning to the new entry dialog box

image

image

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

…n article for manual entry when the fetcher DOI failed to find an entry for an ID [JabRef#7870]
@mrcstan mrcstan changed the title Present options to return to the 'Select entry type' menu or create an article for manual entry ... #7870 Present options to create an article for manual entry or return to the new entry dialog box when failed to find DOI ID ... #7870 Nov 1, 2021
@koppor
Copy link
Member

koppor commented Nov 1, 2021

I really like the idea and the feature. This conforms to our UI strategy (refs JabRef#149).

I think, the feature is not shown in our new "Entry from Id" dialog (see #8129)

grafik

Could you add that feature there, too?

@mrcstan
Copy link
Contributor Author

mrcstan commented Nov 2, 2021

I really like the idea and the feature. This conforms to our UI strategy (refs koppor#149).

I think, the feature is not shown in our new "Entry from Id" dialog (see #8129)

grafik

Could you add that feature there, too?

Thanks for the feedback! I will try to add the feature.

@mrcstan
Copy link
Contributor Author

mrcstan commented Nov 3, 2021

I really like the idea and the feature. This conforms to our UI strategy (refs koppor#149).
I think, the feature is not shown in our new "Entry from Id" dialog (see #8129)
grafik
Could you add that feature there, too?

Thanks for the feedback! I will try to add the feature.

@koppor, here are screenshots of the new feature. Let me know if that is okay
image
image
image

@mrcstan mrcstan marked this pull request as draft November 4, 2021 04:02
@mrcstan mrcstan marked this pull request as ready for review November 4, 2021 04:02
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 6, 2021
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! LGTM from my side. One more approval from another dev and it's good to go

Comment on lines 375 to 378
public StateManager stateManager() {
return stateManager;
}

Copy link
Member

@calixtus calixtus Nov 6, 2021

Choose a reason for hiding this comment

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

This is something to be injected, not to be get. Inversion of dependencies!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I passed the stateManager to the constructors instead of creating a new one

Copy link
Member

Choose a reason for hiding this comment

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

Great!

@calixtus
Copy link
Member

calixtus commented Nov 6, 2021

No, do not create a new one, just inject it in the constructor. just like DialogService or PreferencesService

@koppor koppor merged commit f0f7aa4 into JabRef:main Nov 8, 2021
@koppor
Copy link
Member

koppor commented Nov 8, 2021

@mrcstan Thank you for your contribution! We are look forward to more!

@koppor
Copy link
Member

koppor commented Nov 8, 2021

Side effect - dialog also shown with "Failed" in the title. This is OKish somehow

grafik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants