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

Add button styles to edit-diary-entry-button #3019

Closed

Conversation

tordans
Copy link
Contributor

@tordans tordans commented Dec 27, 2020

This adds a button style and also a bit of spacing.

Before:

Bildschirmfoto 2020-12-27 um 14 26 21 Kopie

After:

Bildschirmfoto 2020-12-27 um 14 26 11 Kopie

Disclaimer: I did not run the code since my setup is not operational ATM.

The style looks different from the comments-buttons (example https://www.openstreetmap.org/user/tordans/diary/395215) but it uses the default bootstrap style so I guess we are moving everything to this style step by step, right?

@tordans
Copy link
Contributor Author

tordans commented Dec 27, 2020

The tests are failing at

Failure:
DiaryEntriesControllerTest#test_edit [/home/runner/work/openstreetmap-website/openstreetmap-website/test/controllers/diary_entries_controller_test.rb:321]:
Expected exactly 1 element matching "a[href='/user/User%202002/diary/16/edit']", found 0..
Expected: 1
  Actual: 0

rails test test/controllers/diary_entries_controller_test.rb:250

I wasn't able to get the dev env running again, yet, so I cannot look into it ATM.

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 27, 2020

:class => "btn btn-primary mt-3" is added as an additional URL parameter to the hyperlink and turns it into http://localhost:3000/user/mmd2/diary/4/edit?class=btn+btn-primary+mt-3, which is probably not what you want. The test fails as it is no longer matching the expected hyperlink.

@tomhughes
Copy link
Member

Yes, if you want HTML options those will need to be in a separate hash.

@tordans tordans force-pushed the add-button-styles-to-edit-button branch from a7937b0 to 418eeff Compare December 28, 2020 08:42
@tordans
Copy link
Contributor Author

tordans commented Dec 28, 2020

I am not sure I needed this journey into the history of Rails URL pattern :-D, but I hope the current version will work.

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 28, 2020

Technically speaking this appears to be working ok now. However, the list of diaries UX seems a bit inconsistent to me now:

pr3019

@gravitystorm
Copy link
Collaborator

Technically speaking this appears to be working ok now. However, the list of diaries UX seems a bit inconsistent to me now:

Ha, I was just taking that same screenshot too!

Copy link
Collaborator

@gravitystorm gravitystorm left a comment

Choose a reason for hiding this comment

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

This seems like a straightforward PR, but unfortunately it opens a can of worms.

As @mmd-osm notes, this proposal breaks the diary list pages, which uses the same partial. I'm not sure what the best solution is here, since I'm happy to move from links to buttons to denote 'actions' (and trying to edit a diary entry seems like an 'action' to me). Perhaps one solution is to use two different partials, and make the diary list a "summary" of each entry (e.g. with a max word limit) rather than including the entire entry. Or alternatively, perhaps we keep the partial just for the summaries, and move the "full display" directly into the view.

Beyond that, one more minor point:

  • The need for pt-3 and mt-3 is not clear. I think the pt-3 is because the location partial is "bare", i.e. text not wrapped in a paragraph, and so I would look into changing the html first, to avoid needing any explicit padding/margins.

@tordans tordans closed this Dec 28, 2020
@tordans
Copy link
Contributor Author

tordans commented Dec 28, 2020

Thanks. Will look into it more once I have a Dev env again (and the time).

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

4 participants