Skip to content
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

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

Tarunmeena0901
Copy link
Contributor

@Tarunmeena0901 Tarunmeena0901 commented Dec 18, 2023

Problem

[BB-343]

Solution

Enhanced main entity editor page:

  • Introduced an identifier editing section.
  • Added a section for adding aliases, improving overall editing flow.
  • Aligned the remove button with identifier rows for a more polished appearance.

Areas of Impact

~ identifier-editor.js
~ identifier-row.tsx
~ alias-editor.js
~ entity-editor.tsx

( u will find the screenshots in comments )

@Tarunmeena0901
Copy link
Contributor Author

i fixed the "add new identifier" in alias editing section as u can see in screenshots

@Tarunmeena0901 Tarunmeena0901 changed the title other(editor): Integrated the modification of identifiers and aliases into the primary editing workflow fix BB-343 : Integrated the modification of identifiers and aliases into the primary editing workflow Jan 2, 2024
Copy link
Member

@MonkeyDo MonkeyDo left a 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
Copy link
Member

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?

Copy link
Contributor Author

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

@Tarunmeena0901
Copy link
Contributor Author

Tarunmeena0901 commented Jan 12, 2024

I understand everyone is very busy after vacation so please take your time for reviewing my PR's 😄
I do wanted to propose a design for this issue but I was waiting for a reply as I was not sure weather am I allow to change the layout or not.......
I agree that alias should be near name section but I think have to think of a clean UI which feels like a part of main editing workflow
so i'll be back with a design proposal

@Tarunmeena0901
Copy link
Contributor Author

Tarunmeena0901 commented Jan 16, 2024

alias

so these are the changes I made

  • changed the position of the alias section
  • reduced the size of fields in both alias and identifier section
  • removed the extra distance between rows of both section and keep it just enough to look clean
  • in last commit you asked about the function onClose , so in this case when we are not using a modal we may not need that button or function so I am removing it in this commit
  • also removed props.aliasEditorVisible & props.identifierEditorVisible from entity-editor.tsx because we are no more managing state of visibility of both editors
  • I tried to merge the alias section with name section but was not able to find any better UI which goes well with our editing flow and also look clean according to current UI . so I kept a different section of alias editing

Screenshot 2024-01-16 181304
Screenshot 2024-01-16 181327

please have a look sir @MonkeyDo

@Tarunmeena0901
Copy link
Contributor Author

also I noticed that unified form is also using same components from entity editor so if we make any changes in entity editor the changes are also visible on /create route . so I think we will also have to make changes here too . please have a look .

unified

@MonkeyDo
Copy link
Member

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).
It might require a more general reworking of the edit page, and as you mentioned it also needs to take the book wizard page into account.

Are you up for the challenge of proposing a mockup for improving the page design so that this new section fits better?
If not I am happy to work on it but it will most likely take me a couple of weeks to get to.

@Tarunmeena0901
Copy link
Contributor Author

It might require a more general reworking of the edit page, and as you mentioned it also needs to take the book wizard page into account.

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
@Tarunmeena0901
Copy link
Contributor Author

Tarunmeena0901 commented Apr 23, 2024

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:

  • I removed the button bar component from the entity editor page as it was no longer needed. However, I simply deleted the folder and files; they still exist.

  • Changes may show that some files were deleted and added, but that's only because of renaming of parent folders. For example, alias-editor => alias-section.

  • I added a new Redux action and case to add new aliases and identifiers, and removed the addAliasRow and addIdentifierRow from Redux.

  • I added a new modal to edit the existing identifiers and aliases.

  • I renamed the identifierEditor and aliasEditor variables in backend and test files to identifierSection and aliasSection, respectively.

If there is any other place where we need to do some renaming please let me know

Screenshot 2024-04-24 035858

Screenshot 2024-04-24 035843

Screenshot 2024-04-23 230944

@MonkeyDo
Copy link
Member

Hello, thanks again for working on this, it's not an easy issue!
I have tested the PR locally, and I think there is some room for improvement in terms of UI and UX.
That being said, I did send you off with basically no direction on that subject, so I blame myself. The coding aspects look fine.

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.
I reckon some of that experimentation I will have to do directly in the code to make it easier, in which case I could possibly directly push my changes if that's OK with you.
Otherwise, If you would prefer to do the changes yourself, I can do my best to give you better mockups and some clear directions for the code if you prefer.
What do you think?

@Tarunmeena0901
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants