-
Notifications
You must be signed in to change notification settings - Fork 3
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: add ui to configuration data exchanges [DHIS2-16434] #70
Conversation
🚀 Deployed on https://pr-70--dhis2-data-exchange.netlify.app |
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 review this in multiple attempts and share what I found so I don't have to keep the tab open for a long time and potentially lose everything
I created a configuration with a single request ("Foo bar baz" on play/dev). When I edit that configuration and delete the request, the request disappears and I can successfully save the configuration. When I then open the configuration again, the request is still there:
className = '', | ||
}) => ( | ||
<div className={className}> | ||
<div className={styles.subtitleContainer} onClick={onTextClick}> |
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.
Needs cursor: pointer;
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 components in src/pages
import directly from the structure inside src/components
. This is something I'd argue should be done through a consisten use of index.js
so pages have to import from src/components
only
return error?.message | ||
} | ||
|
||
export const EditExchange = ({ exchangeInfo, addMode }) => { |
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 name is misleading as it's also used by the add-view, maybe ExchangeForm
or something along those lines is better.
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.
Updated this name and other similar names, now using ExchangeForm, ExchangeFormContents, RequestForm, RequestFormContents
Thanks for the review @Mohammer5 🙏! I've updated to address your points. |
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 didn't find anything obvious anymore, looks good to me so far.
In this case it might be good to get another review on this one as it's so much code that it's quite easy to overlook things. I'll leave that up to you
3e91cd9
to
3539c4b
Compare
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.
approved - could you please add a ticket to look at the tests after hard freeze (also generally the value of using cypress fixtures)
# [100.2.0](v100.1.6...v100.2.0) (2024-02-29) ### Features * add ui to configuration data exchanges [DHIS2-16434] ([#70](#70)) ([96f6063](96f6063))
🎉 This PR is included in version 100.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
See ticket: https://dhis2.atlassian.net/jira/software/c/projects/DHIS2/issues/DHIS2-16434
And figma mockup: https://www.figma.com/proto/7pn74aKmqiaiK2Y52XjwGN/Aggregate-Data-Exchange-Configurator
Note if you are testing, you need to use
system
user (or another user withALL
authority or ADD/DELETE authorities for data exchanges)UI Differences from figma
target
for an external type exchange (this is because you currently need to update authentication details when making these edits)Technical implementation
File
and lets you click to open the file dialog, which is not consistent with the use case here. I think it would be better to work with analytics team to export this from analytics, but there's a couple of other modifications we also will need within this app, so it seemed to just import the code directly for now.Known issues