Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initialize setting review / proposal stage page #782
base: master
Are you sure you want to change the base?
Initialize setting review / proposal stage page #782
Changes from 14 commits
1497d90
f3095e6
8bb98fb
57d036c
e3fcda2
2881ea9
db4fea7
6ef2f5b
211bf3c
048c941
76c39dc
fdd09ab
8ade9c1
cfb5fa1
afaf6d5
88ee598
9ef909e
d579e69
2fb312d
500b9db
ced5b07
928e2d1
32f30a1
c4ef31e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I am having doubts about making this field "required" because I think we should be able to disable it by not providing any value?
And another thing is that the backend needs the format exactly as
YYYY-mm-ddThh:mm:ss
, you need to put the T in there, but the placeholder hints without the T. I personally think thatYYYY-mm-dd hh:mm:ss
should be more user-friendly, and you can consider accepting values withoutss
(treat missingss
as 0).You can take a look at how django implements this here:
https://github.com/django/django/blob/master/django/forms/fields.py#L366
They take multiple possible formats and try one another, described in here https://docs.djangoproject.com/en/3.0/ref/settings/#datetime-input-formats
I think this is a little bit too much, but if you have time you can try this. The bottom line is at least make it more user friendly and the frontend hint needs to meet the backend validation criteria.
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.
Consider accepting values without ss -> 9ef909e
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.
But on the other hand, what does it mean that we should be able to disable it by not providing any value ? Is it for users to change a value individually?
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.
What I meant is that,
proposals.disabled.after
should take effect if this key / value is set. If no value is set for this key, like, an empty value, then it will never be disabled after any time.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.
I think you should read current values from the database and show current settings on page load.
Something like
The above is just a sample code, very ugly, but just a simple demonstration of how django template variables and javscript can be integrated together.
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.
There are currently two ways to display the current settings. The first is that your suggestions are automatically loaded into the form that will be submitted (500b9db). The second is to list the current settings in another list (2fb312d).
But the web page is a bit crowded.
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.
Maybe the two functions of the current review stage status and the set review stage can be presented in different pages ?Do you have more ideas ?
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.
uh... what I mean is that the boolean switches should represent the current set values. So that when the admin sees the page, current settings are clear. And the admin can do some minor tuning without "not knowning" what current values look like.
我想用中文可能比較好表達。原本是不會載入目前的設定的,所以每次進來這一頁都是 false、或者是空的值。這樣其實很難一目瞭然目前到底是哪一個階段,或者是說目前的設定到底是長怎樣。期待的樣子是我進來的時候,所有的值就是目前的設定。舉個很爛的例子,假設目前是 editable、withdrawable 是開的,然後突然要很臨時的開放 creatable 一小段時間,我就只要把 creatable 打開,然後時間過了我再把它關起來。我就不需要再一個一個確認其他某一個的值是什麼,然後小心翼翼的調整怕弄錯,然後改完又要再小心翼翼的改回去。