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

Don't duplicate simple primary keys in the link column #209

Merged
merged 4 commits into from Apr 18, 2018

Conversation

russss
Copy link
Contributor

@russss russss commented Apr 15, 2018

When there's a simple (single-column) primary key, it looks weird to duplicate it in the link column.

This change removes the second PK column and treats the link column as if it were the PK column from a header/sorting perspective.

This might make it a bit more difficult to tell what the link for the row is, I'm not sure yet. I feel like the alternative is to change the link column to just have the text "view" or something, instead of repeating the PK. (I doubt it makes much more sense with compound PKs.)

Bonus change in this PR: fix urlencoding of links in the displayed HTML.

Before:
image

After:
image

@russss
Copy link
Contributor Author

russss commented Apr 15, 2018

I suspected this would cause some test failures, but I'll wait for opinions before attempting to fix them.

@simonw
Copy link
Owner

simonw commented Apr 16, 2018

I think this is a good improvement. If you fix the tests I'll merge it.

russss added 3 commits Apr 16, 2018
When there's a simple (single-column) primary key, it looks weird to
duplicate it in the link column.

This change removes the second PK column and treats the link column as
if it were the PK column from a header/sorting perspective.
@russss
Copy link
Contributor Author

russss commented Apr 16, 2018

Tests now fixed, honest. The failing test on Travis looks like an intermittent sqlite failure which should resolve itself on a retry...

@russss
Copy link
Contributor Author

russss commented Apr 17, 2018

I've added another commit which puts classes a class on each <td> by default with its column name, and I've also made the PK column bold.

Unfortunately the tests are still failing on 3.6, which is weird. I can't reproduce locally...

@simonw
Copy link
Owner

simonw commented Apr 18, 2018

I managed to get a better error message out of that test. The server is returning this (but only on Python 3.6, not on Python 3.5 - and only in Travis, not in my local environment):

{'error': 'interrupted', 'ok': False, 'status': 400, 'title': 'Invalid SQL'}

https://travis-ci.org/simonw/datasette/jobs/367929134

simonw added a commit that referenced this pull request Apr 18, 2018
It was failing intermittently in Travis - see #209
simonw added a commit that referenced this pull request Apr 18, 2018
It was failing intermittently in Travis - see #209
@simonw
Copy link
Owner

simonw commented Apr 18, 2018

OK, aaf59db should mean that the tests pass when I merge that.

@simonw simonw merged commit 136a70d into simonw:master Apr 18, 2018
1 check failed
simonw added a commit that referenced this pull request Apr 18, 2018
This ensures that columns with spaces in the name will still
generate usable CSS class names. Refs #209
@russss russss deleted the cleaner-link-column branch Apr 18, 2018
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