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
THREESCALE-10245: access tokens expiration UI #3943
base: master
Are you sure you want to change the base?
THREESCALE-10245: access tokens expiration UI #3943
Changes from 3 commits
dc05a73
4f4690d
995530a
4036f22
d2f7411
6d88683
14b083d
fda4ff2
667e146
0921951
588b314
587a618
39b8c24
88badaf
bfd0398
62d048d
7f45144
05bc601
6dbcdfe
63f4496
5a7255e
85abc92
6bec27c
25ce970
b4802bf
ee5b4c2
6c597c6
abdbd89
a1511b5
5ada2fa
cca221d
4acb540
96c7097
5425183
4d8a47c
f26aced
3101fe9
231b1f0
c2057e9
d339b14
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.
Removed the suggestion, because noticed that it's not just "if-else", but "if - else if", and value can be empty at the return.
Maybe we can accept both formats on the backend by using
DateTime.parse
? It seems to work both with milliseconds and no milliseconds.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'll explore this possibility, it might work
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.
d2f7411
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 don't love these synthetic ids on the options list. It's a bit hard to understand why this is checked for equality with
5
.Some alternative ideas I had:
or something like that.
Or even remove the
id
, and reuseperiod
, and have for exampleperiod: 0
for "no expiration", andperiod: -1
for custom. This way there are less values that we need to care about.I also suggest to use
label
rather thanname
, as it is used for the fieldlabel
inFormSelectOption
, so we'll have something likeThere 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.
Done fda4ff2
I didn't do this because I find this as confusing as the previous code.
Done 14b083d
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 don't like this break here, but I couldn't find an easy way to add some top margin, so that's OK...
I was looking at https://pf4.patternfly.org/utilities/spacing/, and was hoping that adding
className="pf-u-mt-md"
would help, but it didn't 😬 I think that probably the CSS containing this class is not being loaded... not sure, but I didn't investigate further.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.
667e146
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.
Also, now I think the parenthesis
()
are not needed, but well, not important.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.
This is the kind of stuff that belongs to the presenter IMHO. Also, it's not recommended to use instance variables in partials, pass it as a local instead.
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.
The form is loaded from
#new
and#edit
actions, and they would probably need a different presenter each. I didn't move this logic to a presenter to avoid creating a new presenter for #edit and having to duplicate a new methodaccess_token_persisted?
in both presenters.I moved the instance var to a local, though: 4acb540