-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat(FieldFormatters): add visual editor #5075
base: production
Are you sure you want to change the base?
Conversation
if (mapping !== undefined && mapping?.length > 1) | ||
softError('Expected mapping length to be no more than 1'); | ||
const field = mapping?.[0]; | ||
if (field?.isRelationship === true) { | ||
softError( | ||
'Did not expect relationship field in field formatter mapping' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these cases should not happen - the UI doesn't allow them. but handling them here just in case (and to satisfy TypeScript)
specifyweb/frontend/js_src/lib/components/SchemaConfig/Format.tsx
Outdated
Show resolved
Hide resolved
❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- When creating a field format like
GiftNumber
using the regular expression/^(KUI|KUBI|NHM)$/
, I am unable to set it up correctly.
See that I am supposed to put the 'hint' on the left and 'pattern' on the right:
This works as expected when using a regex validator, but when setting it as the pattern it is not recognized properly.
<format system="true" name="GiftNumber" class="edu.ku.brc.specify.datamodel.Gift" fieldname="giftNumber" title="" default="true">
<autonumber>edu.ku.brc.af.core.db.AutoNumberGeneric</autonumber>
<field type="regex" size="4" value="/^(KUI|KUBI|NHM)$/" byyear="true" pattern="KUBI"/>
</format>
- After modifying other sections of the format, those changes are not made immediately visible for the 'Example Field'. Instead, it uses the previous field format before the edits have been made.
See that it shows "Required Format: KUBI" in the tooltip:
(This tooltip is most often obscured by the browser's "match the requested formatter" error)
https://github.com/specify/specify7/assets/37256050/d34bcb16-85d5-43fc-8c68-56bef2e36cc6
After making it simply numeric:
- What does the 'Auto Numbering' do that the 'Auto-number' checkbox does not?
Looks like the expected regex between Sp6 and Sp7 differed. Fixed:
In practice, that means:
|
yeah, it's not ideal that 'hint' comes before the 'pattern' as the opposite order seems more intuitive. The reason I did it this way is because of the column heading. Open to ideas on how to make this more intuitive |
Fixed |
Ups, the "Auto Numbering" checkbox at the top is not supposed to be exposed in the UI - removed |
@maxpatiiuk Can you resolve the merge conflicts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing instructions
Generally visual editor for Field Formatters should act and look very similar to the one for Record formatters since it shares a lot of code with it.
To test:
- Verify that each field formatter type (constant, any character, ...) can be set in the UI, without user-experience issues, and after saving, can be read back correctly
- Verify that a formatter created/edited using a visual editor works in Specify 7 and Specify 6 (in forms and query results)
- For reference, here is how to configure a field formatter in Specify 6
- P.S: don't try to enter an emoji character like on the screenshot above - Specify 7 back-end does not support emojis in app resources 😢
Super exciting! I have a few recommendations:
- If no default formatter is selected, the message says "record formatter. I would change this to "field formatter"
- The default for the "size" field for most field types is 0, and this can be saved as long as there is a value in the "hint" field. The value will be shown in the formatter, but it will make the format invalid if that value is entered in the field. In the Specify 6 field formatter, the "size" value cannot go below 1; I think adding this in 7 would be a good solution.
- The user can input and save any value at any length in the "hint" field (see screenshot above) for the numeric field type, but this is misleading because "#" is the only character that actually works, and it will only appear as many times as the value in the "size" field. Maybe this field could be read-only since it's not really functional.
I'd like to hear what others have to say about 2 and 3; they don't conflict with the xml editor, and the "Example field" displays their consequences correctly, but I think these solutions would make the visual editor more user-friendly.
Good point. To have the same message be applicable both in the field formatters editor and record formatters editor, I edited it to say: "Please designate one of the formatters as default"
Made "size" value not go below 1
Fixed. "hint" is now readonly for number fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Another thing I noticed is that clicking on the "edit" button next to a format in schema config often takes you to the wrong one.
Schema.Config_.Gift._.Specify.7.-.Google.Chrome.2024-07-23.11-30-30.mp4
https://fwri1924-field-editor.test.specifysystems.org/specify/schema-config/en/Gift/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation of the types it was very helpful! I think 'value' instead of 'hint' would make more sense as it is likely what the users are more familiar with but I am open to other opinions @grantfitzsimmons
Set max size to 9999.
This works technically but it messes up the dialog, is there a reason the size would need to be this big in the first place? Also you can keep typing in the size field above 9999 and eventually crash the browser still, it just turns red if you click out of it.
chrome_3c0bt3oDfT.mp4
chrome_BjqkqNtr4f.mp4
The record being editable is useful in the Record formatter and Web Link formatter as there the value could be coming from a different field (or multiple fields) and so it is useful to be able to edit different record fields and see how they affect the formatted result/weblink url
I thought this issue was fixed but I was thinking of #4891 which stopped you from editing/creating new records in form definitions, however, #4795 is the open issue for fixing this behavior in record formatters and has not been fixed yet. I get what you're saying though and I think a discussion should be had on if we want users to be able to edit forms this way or not.
Information is still carried over when you switch the type, it does change when you select year or numeric but the other ones still carry over. This mostly just seems like a problem because form some of the types you can't change the size but when the info is carried over the size will just be whatever it was for the previous type.
chrome_X5iAzCME8c.mp4
New issues:
If you have the size field selected and scroll up and down the number in the field changes. This is possibly #4245 resurfacing.
chrome_rUOuWstC95.mp4
Not sure if this is supposed to happen but when I set a numeric type to auto-incrementing and put it on the fax field and tried to actually use and save on the form I got an error. Assuming this is intentional, we should probably try to find another solution other than the user getting an error in the forms (maybe some sort of save blocker in the field formatter dialog?)
chrome_oLsql8EBHn.mp4
Specify 7 Crash Report - 2024-12-16T15_51_26.733Z.txt
When typing in the hint field for some of the types it changes the size depending on how many characters you type but for others it doesn't, is this the expected behavior?
chrome_8jSNXvMFcI.mp4
This behavior is fine but is there a reason why clicking the add button in schema config just takes you to app resources instead of pulling up a dialog in schema config?
chrome_PjFpoNxEUt.mp4
The existing system formatters can have multiple formatters be defaults for one table but for new ones only one can be default. This is not necessarily a problem as no errors are thrown or anything but I don't know if it could cause any problems.
chrome_FFF3EwuDdE.mp4
The description for field formatters in the resource says:
And I think this is pretty good but still a little confusing, could it be a little similar to how the description is for table formats/aggregations to also keep things consistent?
I think we should consider removing some tables from the field formatters because right now you can add one to any table. I can see this causing problems but I might be missing some potential use cases.
Same thing with some of the fields. You can add a formatter to the GUID field which should be autogenerated and it just causes a lot of problems.
chrome_78SK4Pd6sb.mp4
- Field is ambiguous name - Field suggests that you can have fields inside of a formatter - you can't - you can only have text parts inside a field formatter
Triggered by 06b3c1a on branch refs/heads/field-editor
Triggered by b4736f8 on branch refs/heads/field-editor
Fixes #5075 (review) I should have known better than to use `useEffect` for this. Moved the logic to a more appropriate lace.
Sounds good. Renamed to "Value"
Good point. Will reduce to 99. We can increase it back if we receive feedback (or people can edit it directly in XML)
Doesn't seem to be a problem - the size is reduced as needed when switching for example from numeric to year: in.mov
Looks like that bug was fixed for floating point fields, but not for integer fields - fixed. Thanks for catching!
Can reproduce.
Expected. For constant parts, the "Value" is literally the exact and only value that can be put into that part - so the size must equal the "Value".
An inconsistency! Fixed
It may cause inpredictability/inconsistency as to which will actually be the default (as only one will be used by specify by default). The same issue is present in Record formatters. The way I decided to handle them there is:
If you wish I can make it more proactive, and display a warning message if multiple defaults are present. For now, we warn in the opposite case:
Made it more consistent. Do say if you have more feedback
Good point. I excluded all read only and virtual fields from the list (for example CatalogNumber.age) |
30ea548
to
8feb53f
Compare
8feb53f
to
454d466
Compare
From #5075 (review) and #5075 (comment) Yes, looks like auto-numbering right now is only supported for tables with hierarchy fields. Specifically for auto-numbering, the backend uses the specify7/specifyweb/specify/uiformatters.py Lines 32 to 33 in 1d6b1ee
The function has support for a The potential consequence of doing so is that if there are two or more records which are supposed to have differing scopes, they will share a global autonumbering scheme. Anything besides making the scoping less strict will require some implementation of #5044 (review)
I think Address is a really great example to consider scoping for auto-numbering (and other applications)! Building on this, Address also has independent relationships in the form of:
Because of the dependent relationship of In the case of the independent relationships, should the Division addresses be considered separately from the Institution/InstitutionNetowork addresses? That is, if we add a new Division, do we only consider the addresses of other Divisions or just all addresses in general? (which might be the same in this case, but might not always be the case across all relationships). A global approach is by far the easiest to implement and think about, at least from a developmental and design standpoint. |
Fixing an issue from 2012 😊
Fixes #23
Checklist
and self-explanatory (or properly documented)
Testing instructions
Generally visual editor for Field Formatters should act and look very similar to the one for Record formatters since it shares a lot of code with it.
To test:
Documentation on field formatters