-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[tags] Improve handling of values #4589
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
Conversation
cc0a832 to
912e4cc
Compare
c6bd5da to
3b0cc83
Compare
|
The lint rule is wrong, and its saying this an error where its missing keys. We can either fix the lint rule (read: its not an error), or i can exclude it entirely in these files. However if we have to exclude it like that it tells me we should change the lint rule, to at best be a warning. |
mattrobenolt
left a comment
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.
python seems ok
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.
Not a big deal, but I had to look up the API for this since the name didn't match what this actually was and couldn't tell if it was meant to be an instance of a class or a class itself with the variable being named _cls.
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.
its the class of serializer, not the literal class type
3b0cc83 to
59764e6
Compare
benvinegar
left a comment
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.
Seems fine, I have some suggested improvements but otherwise ...
| <Avatar user={user} size={96} gravatar={false} /> | ||
| </div> | ||
| <KeyValueList data={builtins} isContextData={false} /> | ||
| <table className="key-value table"> |
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.
So, I see why you removed KeyValueList – because it wouldn't render your custom Email JSX above properly.
I feel a better solution is to modify KeyValueList so that if value is a React component, it should just spit out that component as-is (whereas primitives go through the existing path through deviceNameMapper).
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.
that sounds like a lot of 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.
I can do it in a follow-up.
| return item.ipAddress; | ||
| else | ||
| return item.value; | ||
| }, |
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.
Why not just:
return item.email || item.username || item.identifier || item.ipAddress || item.value;ed98f1e to
a8d60ff
Compare
Security concerns found
Generated by 🚫 danger |
- Add export to csv action in UI - Remove uniqueValues reference (not issue specific) - Augment data when user tag is selected - Add email action for users
a8d60ff to
2ad172d
Compare

Uh oh!
There was an error while loading. Please reload this page.