-
-
Notifications
You must be signed in to change notification settings - Fork 285
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
fix BB-343 : Integrated the modification of identifiers and aliases into the primary editing workflow #1043
base: master
Are you sure you want to change the base?
Conversation
i fixed the "add new identifier" in alias editing section as u can see in screenshots |
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.
Thanks for opening a PR @Tarunmeena0901 !
Sorry it took so long to get to reviewing it.
Currently, my main feedback looking at the screenshot you posted is:
- each row is taking too much space now, both vertically and horizontally; it made some sense in the modals but in the page we need to conserve some space. For example the giant remove button is unnecessary and can be a small trash icon instead; there is too much padding in between rows; the inputs are quite large, etc.
Maybe the sections do not need to take the whole width of the page; I think both would do better with half of the page width (on desktop). - the alias section I think would make more sense next to the name section; after all they are alternate names, it seems odd to me to move that section to the bottom of the page. Looking at the name section component, you might need to refactor the layout if you want to move things around or put the two sections side by side.
* @param {boolean} props.show - Whether or not the editor modal should be | ||
* visible. | ||
* @returns {ReactElement} React element containing the rendered AliasEditor. | ||
*/ | ||
const AliasEditor = ({ | ||
languageOptions, | ||
onClose, | ||
show | ||
onClose |
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 name onClose needs to be changed everywhere since the function has changed.
What does it do now?
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.
ok so this onClose is a dispatch function which is used to remove any empty alias row when user click on Done button (refer SS),
I thought to change the name to onDone but it doesn't seem very descriptive to me so I thought it may be good to hear some review
src/client/entity-editor/identifier-editor/identifier-editor.js
Outdated
Show resolved
Hide resolved
src/client/entity-editor/identifier-editor/identifier-editor.js
Outdated
Show resolved
Hide resolved
I understand everyone is very busy after vacation so please take your time for reviewing my PR's 😄 |
so these are the changes I made
please have a look sir @MonkeyDo |
Sorry for the delay on reviewing this; I think it deserves a bit more work in terms of design, I think the identifiers section as laid out currently takes a lot of space (I think this was most likely the reason why it was put in a modal before). Are you up for the challenge of proposing a mockup for improving the page design so that this new section fits better? |
I guess I'll have to sit down with my pencil paper and think this out properly. This week is pretty loaded, but next week won't be. I'll ping you soon... |
…to identifierSection and aliasSection respectively
Apologies for taking so long to implement the mockups; there was more work than I expected. The changes in Git may seem confusing, so I'll mention some main changes I made:
If there is any other place where we need to do some renaming please let me know |
Hello, thanks again for working on this, it's not an easy issue! I need to sit on it for a bit and do some experimentation to come up with a more detailed mockup of what the pages should look like. |
If you have specific mockups or ideas in mind, I am more than happy to implement them. However, if you find it easier to experiment directly with the code, please feel free to push your changes to the PR. Given that the GSoC coding period is ongoing and you might have a lot on your plate, please let me know how you'd prefer to proceed. I'm open to either option. |
Problem
[BB-343]Solution
Enhanced main entity editor page:
Areas of Impact
~ identifier-editor.js
~ identifier-row.tsx
~ alias-editor.js
~ entity-editor.tsx
( u will find the screenshots in comments )