evidence icon indicating copy to clipboard operation
evidence copied to clipboard

HTML select is out of order in Dropdown

Open archiewood opened this issue 2 years ago • 14 comments

Steps To Reproduce

Nav to https://mdsinabox.com/nba/historical_matchups/

Open the picker for Year

NB edited screenshot

CleanShot 2024-01-10 at 14 21 25@2x

Expected Behavior

years should be in order because they are sorted in the query

Actual Behaviour

years are not in order

archiewood avatar Jan 10 '24 19:01 archiewood

image

Seems intentional based on the source markdown

We should support default=1986 though, which seems like the goal of this, might be worth adding on to #1477

csjh avatar Jan 10 '24 19:01 csjh

This screenshot was suboptimal for showing the issue

Here is where it gets really weird

CleanShot 2024-01-10 at 14 21 25@2x

archiewood avatar Jan 10 '24 19:01 archiewood

Looks like filtering with SELECT DISTINCT throws ordering out the window, not sure if there's a good way for us to preserve (repro w/ SELECT DISTINCT range AS blah FROM (SELECT * FROM (SELECT * FROM range(1, 100) ORDER BY range DESC));)

Adding order=season on the Dropdown should fix

csjh avatar Jan 10 '24 19:01 csjh

@matsonj

archiewood avatar Jan 10 '24 19:01 archiewood

agree with having a default.

I think we should also have this default for the ButtonGroup

archiewood avatar Jan 10 '24 19:01 archiewood

SELECT DISTINCT range AS blah FROM (SELECT * FROM (SELECT * FROM range(1, 100) ORDER BY range DESC));

I guess an option is for us to not do any DISTINCT-ing

archiewood avatar Jan 10 '24 22:01 archiewood

Yeah that's also an option, just not sure which is more important between distinct vs. preserving order

csjh avatar Jan 11 '24 13:01 csjh

Not 100% sure either but I would lean towards preserving order over distinct-ing:

Any SQL building that we do on top of the queries can lead to inexplicable results. I was on a call with a user who couldn't work out why their sort was discarded.

However if you write a query with no group by / distinct and your dropdown has multiple entries for each option, your steps to rectify are clear.

An option would be to have a distinct=true prop that you add to the Dropdown componenent

archiewood avatar Jan 11 '24 14:01 archiewood

I think the key bit from a DX perspective is that SQL should be transparently passed to components. It should be consistent for me to reason about when passing SQL to evidence components. So I vote for removing the DISTINCT and adding an option, although unclear why we need distinct in the dropdown component when we can add it in SQL.

matsonj avatar Jan 11 '24 16:01 matsonj

Makes sense, I think the DISTINCT default made more sense when the query was built fully in-component (with table name passed in to the data field instead of a query result)

csjh avatar Jan 11 '24 16:01 csjh

although unclear why we need distinct in the dropdown component when we can add it in SQL.

Yeah I think I agree. It would only be an alternative, but SQL > New syntax a user has to learn

archiewood avatar Jan 11 '24 16:01 archiewood

I am probably misunderstanding something, but can't the "order by" be done after the distinct ?

Or is the issue that the "order by" comes from the client query while the "distinct" is added by evidence? In that case I would also be in favor of removing the distinct added by evidence (the user can still do it) and keeping the order by.

maartenbosteels avatar Mar 08 '24 15:03 maartenbosteels

Yes, the issue is that the order comes from the client query and the distinct is evidence

csjh avatar Mar 08 '24 16:03 csjh

My suggestion is we remove the distinct for a future release.

It shouldn't really be a breaking change, some users passing in non-distinct tables may end up with dropdowns with repeated entries, but the dropdowns would still work, and they could filter them in the SQL to rectify when they notice.

archiewood avatar Mar 08 '24 16:03 archiewood