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 Sep 6, 2022
@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 Sep 7, 2022
…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.

mdelez
mdelez approved these changes Sep 7, 2022
Copy link
Collaborator

@mdelez mdelez left a comment

works well, thanks :)

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