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.

Sign up for GitHub

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

Jump to bottom

Groups: Searching for keywords field mandatory value not checked #6110

Merged
merged 3 commits into from
Mar 31, 2020
Merged

Groups: Searching for keywords field mandatory value not checked #6110

merged 3 commits into from
Mar 31, 2020

Conversation

stefan-kolb
Copy link
Member

@stefan-kolb stefan-kolb commented Mar 13, 2020 •
edited
Loading

Closes #6108
Closes #6066

  • Alert dialog triggered
    image

  • Active Info icons:
    image

  • Inactive info icons
    image

  • Dialog should not close when error dialog is discarded!
    @calixtus Not sure if we have this problem in more dialogs. The setResultConverter method will always close the dialog. We want to go abck to the dialog if errors are displayed as alert dialogs , however. We need to use addEventFilter for this I guess (am no expert in JavaFX @tobiasdiez) See Dialog validation section at https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/Dialog.html

@stefan-kolb stefan-kolb mentioned this pull request Mar 13, 2020
Closed
@stefan-kolb stefan-kolb marked this pull request as ready for review March 13, 2020 16:39
@Siedlerchr
Copy link
Member

In other dialogs we don't show any error message, instead we disable the ok buton until all required fields are set
The dialog is duplicate. See for example the SharedDatabaseLogin Dialog

@JabRef JabRef deleted a comment from codecov bot Mar 13, 2020
@koppor
Copy link
Member

koppor commented Mar 13, 2020

@Siedlerchr I really like that metaphor. Think, it's also in place in Angular apps ^^. Refs koppor#410 (which states that we should document it).

@stefan-kolb
Copy link
Member Author

I agree. But that's not what I fixed and what the PR is about ;-)

@tobiasdiez
Copy link
Member

I agree that the OK-button should be disabled in case there are validation errors. The code for this seems to be in place as well, so I guess there is a bug somewhere. @Siedlerchr @calixtus do you have an idea where?

@calixtus
Copy link
Member

calixtus commented Mar 13, 2020 •
edited
Loading

I did take a look at the code and tried a little bit around, but I must confess, I don't ever reach the alert dialog. The button disableProperty is with this PR now completly and correctly "bound" to the validation status and I'm unable to click the button. So... huh?
Also I agree to the button-disabled-metaphor.

@Siedlerchr
Copy link
Member

So simply remove the dialog and all is done.

@stefan-kolb
Copy link
Member Author

Disabled button should work, except for when the dialog is opened at first. The checks only come into place when one has entered data in the mandatory fields. We need to trigger the validation after initialization to change this.

@stefan-kolb stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 15, 2020
@stefan-kolb
Copy link
Member Author

Can we track the additional changes in a follow up issue? I'd rather limit this PR to the fixes if possible.

@koppor koppor mentioned this pull request Mar 31, 2020
Closed
@JabRef JabRef deleted a comment from codecov bot Mar 31, 2020
@koppor koppor merged commit e866106 into master Mar 31, 2020
@koppor koppor deleted the keyword-group-validation branch March 31, 2020 16:06
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 31, 2020
koppor pushed a commit that referenced this pull request Jul 1, 2022
Squashed 'buildres/csl/csl-styles/' changes from e740261..3d3573c
ffe1663 
3d3573c Update centre-de-recherche-sur-les-civilisations-de-l-asie-orientale.csl (#5988)
5de0fbe Update society-of-biblical-literature-fullnote-bibliography.csl (#5913)
04b6c7a Create revue-internationale-durbanisme.csl (#5974)
4a5bfe2 Update biological-reviews.csl (#6116)
957b2bc Update harvard-cite-them-right-no-et-al.csl (#6115)
e836a6c Update harvard-university-of-bath.csl (#6011)
b4a8dd7 Update and rename harvard-cite-them-right.csl to harvard-cite-them-ri… (#6113)
a198884 Update twentieth-century-music.csl (#6110)
81c1619 Update archaeonautica.csl (#5928)
fc46f1d Bump actions/cache from 2 to 3 (#6112)
fab57ed Bump actions/checkout from 2 to 3 (#6111)
519d594 [don't merge] chore: Included githubactions in the dependabot config (#6109)
a8aa898 Update universidade-estadual-de-alagoas-uneal-abnt.csl (#5915)
6191640 Update isnad-dipnotlu.csl (#5909)
d65a6ac Update isnad-metinici.csl (#5910)
830d337 Update technische-universitat-dresden-linguistik.csl (#6097)
81adc43 Update american-society-for-horticultural-science.csl (#6089)
b767623 Create south-african-law-journal.csl (#6092)
215e1e9 Create journal-of-lithic-studies.csl (#6080)
0740f8c Create eunomia-revista-en-cultura-de-la-legalidad.csl (#6095)
f93c809 Create endocrine-journal.csl (#6086)
3fdeb51 Revert "chore: Set permissions for GitHub actions (#6096)" (#6108)
35ebd1e chore: Set permissions for GitHub actions (#6096)
1cb8758 Create journal-fur-medienlinguistik (#6100)
f4b5f7f Update unified-style-sheet-for-linguistics.csl (#6098)
c3f856a Update advanced-materials.csl (#6103)
d1e7576 Bump diffy from 3.4.0 to 3.4.2 (#6107)
9e5e7ab Fix Dev Dynamics (#6099)
7234520 Add CSL style for the journal Developmental Dynamics (#6093)
ba8db05 Create independent style for vox-sanguinis.csl (#6085)
845dee0 Create meta.csl (#6088)
684bc3a Update universite-du-quebec-a-montreal.csl (#6087)
3602c18 Up-date & re-title pour-reussir/dionne (#6043)
0cc6e82 Fix Mainz Geography
cfc4cec Add DOI and fix printing author names in Population and Économie et statistique (#6079)
14e8b1d Update journal-of-neuroimaging.csl (#6084)
2c0e1f1 Update isnad-dipnotlu.csl (#6081)
02fdb9b Merge pull request #6082 from denismaier/patch-ube-muwi-note
9309378 removes default-locale

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 3d3573c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calixtus calixtus Awaiting requested review from calixtus

@tobiasdiez tobiasdiez Awaiting requested review from tobiasdiez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@stefan-kolb @Siedlerchr @koppor @tobiasdiez @calixtus