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(resource): fix properties sorting (DEV-1204) #818

Conversation

mpro7
Copy link
Collaborator

@mpro7 mpro7 commented Sep 6, 2022

resolves DEV-1204

@mpro7 mpro7 requested a review from mdelez as a code owner September 6, 2022 14:27
@mpro7 mpro7 changed the title fix(resource): fix properties sorting fix(resource): fix properties sorting (DEV-1204) Sep 6, 2022
@mpro7 mpro7 self-assigned this Sep 7, 2022
src/app/workspace/resource/resource.component.ts Outdated Show resolved Hide resolved
src/app/workspace/resource/resource.component.ts Outdated Show resolved Hide resolved
src/app/workspace/resource/resource.component.ts Outdated Show resolved Hide resolved
@mpro7 mpro7 requested a review from mdelez September 7, 2022 08:55
…not-shown-in-the-same-ordering-in-chrome-and-firefox
@mdelez
Copy link
Collaborator

mdelez commented Sep 7, 2022

@mpro7 The if condition in the second sort seems to never be met so I still have the issue on my end. Is it working for you?

@mpro7
Copy link
Collaborator Author

mpro7 commented Sep 7, 2022

@mpro7 The if condition in the second sort seems to never be met so I still have the issue on my end. Is it working for you?

It's working for me. Why do you say the condition it's never to be met?

@mdelez
Copy link
Collaborator

mdelez commented Sep 7, 2022

@mpro7 The if condition in the second sort seems to never be met so I still have the issue on my end. Is it working for you?

It's working for me. Why do you say the condition it's never to be met?

The account I'm using (root) has german set as its language so the label is "hat Standoff Link zu" which is the reason why the if statement is never met. Also, this would mean that if anyone were to put "has" in their property label (which is pretty common unfortunately, especially for Lausanne) it would also get sorted to the bottom.

The biggest issue I see is that these default props do not have a guiOrder. I took a look at a more complex resource to see what else we can sort by. I think we're safe to either sort all properties that have a undefined guiOrder to the bottom or we can sort by isEditable but I'm not certain under what conditions this boolean is set.

@mpro7
Copy link
Collaborator Author

mpro7 commented Sep 7, 2022

@mpro7 The if condition in the second sort seems to never be met so I still have the issue on my end. Is it working for you?

It's working for me. Why do you say the condition it's never to be met?

The account I'm using (root) has german set as its language so the label is "hat Standoff Link zu" which is the reason why the if statement is never met. Also, this would mean that if anyone were to put "has" in their property label (which is pretty common unfortunately, especially for Lausanne) it would also get sorted to the bottom.

The biggest issue I see is that these default props do not have a guiOrder. I took a look at a more complex resource to see what else we can sort by. I think we're safe to either sort all properties that have a undefined guiOrder to the bottom or we can sort by isEditable but I'm not certain under what conditions this boolean is set.

Hmm, the language factor I haven't took into the consideration. That's why the property labels should be unified and in English only ;)

Anyways, I will try to change the condition and see how if it works with undefined. It seems to be the most reasonable.

Copy link
Collaborator

@mdelez mdelez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works well, thanks :)

@mpro7 mpro7 merged commit fbee603 into main Sep 7, 2022
@mpro7 mpro7 deleted the DEV-1204-dsp-app-has-incoming-link-property-not-shown-in-the-same-ordering-in-chrome-and-firefox branch September 7, 2022 11:36
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.

None yet

2 participants