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

Button enhancement #48

Merged
merged 6 commits into from
Oct 25, 2019

Conversation

sevenseas-gists
Copy link
Contributor

closes #45 #46

@codecov-io
Copy link

codecov-io commented Oct 2, 2019

Codecov Report

Merging #48 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #48      +/-   ##
===========================================
- Coverage      1.98%   1.98%   -0.01%     
  Complexity      225     225              
===========================================
  Files            19      19              
  Lines          1413    1414       +1     
===========================================
  Hits             28      28              
- Misses         1385    1386       +1
Impacted Files Coverage Δ Complexity Δ
block_evasys_sync.php 0% <0%> (ø) 37 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f8b0b7...145ce9e. Read the comment docs.

@Dagefoerde
Copy link
Member

Seeing that the course mapping button does not submit a form, and is rather a link, it could look a bit more lightweight. Have a look at the screenshot below for an example (blue links in the top right corner):

new_ess

Here's similar code for reference: https://github.com/unirz-tu-ilmenau/moodle-block_opencast/pull/95/files#diff-ecf144e33d13e81a79f4124b4526e3e3R29

@tobiasreischmann
Copy link
Member

Seeing that the course mapping button does not submit a form, and is rather a link, it could look a bit more lightweight...

However, we are not in a page context here. We only have the space of a block. Would you suggest to place this blue lightweight link in the right top corner of the block? (I would vote against that)
Or do you want to simply replace the button by this link at the same position?

@tobiasreischmann
Copy link
Member

@rtschu If you make layout changes, it would be very nice to have a previous/after through screenshots. This way, the peer reviewer do not have to setup a working installation with dummy data, but can solely look at the code and the visual result.
In general the PRs can have more descriptions. Links to related issues are helpful. But you should also add a description what has changed to the PR itself. Looking through the linked issues is tedious.

@Dagefoerde
Copy link
Member

Seeing that the course mapping button does not submit a form, and is rather a link, it could look a bit more lightweight...

However, we are not in a page context here. We only have the space of a block. Would you suggest to place this blue lightweight link in the right top corner of the block? (I would vote against that)
Or do you want to simply replace the button by this link at the same position?

The former is exactly what I meant (as this kind of link is in the top right corner of other pages as well). I vote in favor of that because it corresponds to how things are displayed that are part of pages.

@tobiasreischmann
Copy link
Member

Ok. We dicussed: Can you alter it to be a link as suggested by @Dagefoerde. It should however remain at the same position.

@Dagefoerde
Copy link
Member

Please see #52 (review). Also, is there an update on the placement of the link?
Thanks for making the changes and for all your work!

Copy link
Member

@Dagefoerde Dagefoerde left a comment

Choose a reason for hiding this comment

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

(see comments)

@sevenseas-gists
Copy link
Contributor Author

Did you look for something like this?
Screenshot_2019-10-18 Kurs Test 3

@Dagefoerde
Copy link
Member

Exactly, but it would be nicer without the underline. You would achieve that by using the CSS classes that were used in the example code that I linked.

@tobiasreischmann
Copy link
Member

This is the current view:
image

Copy link
Member

@Dagefoerde Dagefoerde left a comment

Choose a reason for hiding this comment

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

You could remove the <div class="pl-1"> ... </div> (meaning padding-left: 1px), as they are only useful if there is a sequence of links.

<div class="pl-1"> ... </div> (meaning padding-left: 1px),
thus they are only useful if there is a sequence of links.
@Dagefoerde
Copy link
Member

:shipit: