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

Convert the "Custom API key" list to a table [#10926] #10936

Merged
merged 11 commits into from
Mar 4, 2024

Conversation

AbdAlRahmanGad
Copy link
Contributor

@AbdAlRahmanGad AbdAlRahmanGad commented Feb 28, 2024

Fixes #10926

demo.mov

The only difference from the previous implementation that the user can edit the "key" even if the "use custom key" is not checked. I still didn't find an efficient way to solve it so I will wait for your feedback.

Mandatory checks

  • 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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • 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.

@calixtus
Copy link
Member

Hi, thanks for your interest in JabRef and OpenSource. I like the change to the ui, yet i have a comment about your implementation. I recommend the book Effective Java by Josh Bloch. Here is a summary that I like to use myself: https://github.com/HugoMatilla/Effective-JAVA-Summary

@AbdAlRahmanGad
Copy link
Contributor Author

@calixtus Hi, thanks for the feedback. I will check the resources you provided and modify the code.

@Siedlerchr
Copy link
Member

You could disable the field if it's not selected (e.g binding)
And regarding the exception handling, better is to use Validation

@calixtus
Copy link
Member

Shouldn't the keys be hidden behind asterisks
@JabRef/developers ?

@Siedlerchr
Copy link
Member

Yeah, maybe some kind of PasswordField or something similar can be used

@koppor koppor mentioned this pull request Feb 28, 2024
6 tasks
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

How can I change a key? I tried to click and double click (Windows 11), but nothing happened.

The checkbox on the right should be enabled only if there is a custom key.

image

@AbdAlRahmanGad
Copy link
Contributor Author

@koppor I made that you can't enter a key unless you enable the check. 0914142

Should I make it the opposite: if you enter a key then the checkbox will be enabled?

@koppor
Copy link
Member

koppor commented Feb 29, 2024

Yeah, the opposite is great. The checkmark should automatically be set then.

Maybe no disablement of the checkbox. There seem to be two kinds of user flows 🤣🤣

On dialog close, the key should only be enabled if non empty.

@AbdAlRahmanGad
Copy link
Contributor Author

@koppor Just to make sure I understood right.

  • If a user enter a key the checkbox is enabled automatically
  • if the key is empty the checkbox is disabled automatically

Also, can the user change the checkbox manually?

@koppor
Copy link
Member

koppor commented Feb 29, 2024

@koppor Just to make sure I understood right.

You came up with an even better solution!

Go for it!

@calixtus
Copy link
Member

Until now it was also possible to store custom keys, even if they are not used. Would be a regression if we automatically use the key if stored or remove it, when unchecked...

@AbdAlRahmanGad
Copy link
Contributor Author

@calixtus Sorry, I saw your comment after I commited.
So, should I make the user able to edit both manually?

@calixtus
Copy link
Member

calixtus commented Mar 1, 2024

There are use cases one would want to keep the custom API stored without using it, maybe just to test something maybe.

@AbdAlRahmanGad
Copy link
Contributor Author

I think the code is ready for review now.

@AbdAlRahmanGad AbdAlRahmanGad requested a review from koppor March 2, 2024 15:13
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 3, 2024
@Siedlerchr
Copy link
Member

From my side lgtm, just one minor addition: Can you add the note that is at the tab Citation key patterns, here as well?

grafik

Siedlerchr
Siedlerchr previously approved these changes Mar 3, 2024
@Siedlerchr Siedlerchr enabled auto-merge March 4, 2024 19:43
@Siedlerchr Siedlerchr added this pull request to the merge queue Mar 4, 2024
Merged via the queue into JabRef:main with commit 8f46463 Mar 4, 2024
20 checks passed
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.

Custom API keys should be a table
4 participants