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

Fixed issue with focus on the book images #1496

Merged
merged 2 commits into from
Apr 21, 2021
Merged

Conversation

Goodiec
Copy link
Contributor

@Goodiec Goodiec commented Apr 9, 2021

Issue Description

Noticed an issue in the Books column on the learn page. Clicking on the first image applies the style for focus on the second image. Clicking on the second image applies to the second and also the outline from the style displays off the outline of the images. This issue is shown in the images below

Please include a summary of the issue.
The style for focus wasn't displaying properly for the book images.

Fixes # (issue)
Fixes #1357

Changes Made

Added a class to the affected images and then used CSS in the CSS file to fix the issue.

Please describe the changes that you made.
Remove the inline styling that was added to both images that were contributing to the issue and added new classes to display the images properly.
Before

before-focus
before-focus1

After

afterfocus-2
afterfocus-2 1

  • Please check if the PR fulfills these requirements
  • PR is descriptively titled and links the original issue above
  • Before/after screenshots (if this is a layout change)
  • Details of which platforms the change was tested on (if this is a browser-specific change)
  • Context for what motivated the change (if this is a change to some content)

@Goodiec
Copy link
Contributor Author

Goodiec commented Apr 9, 2021

@pitag-ha, @gs0510, I Just noticed that the contents from the books section down on the docs page of the ocaml.org website do not get displayed properly on smaller screen sizes, there's way too much empty space on the right. Can I add a commit to this request to fix that or should I open an issue for it?

doc2
doc1

@nibble0101
Copy link
Contributor

Hi @Goodiec.

Thanks for the PR. I tried going through the changes you made but looks like some of them are a result of formatting the code. Is that correct? If so, It would be best to always avoid committing formatting changes otherwise reviewing your code becomes difficult.

@Goodiec
Copy link
Contributor Author

Goodiec commented Apr 9, 2021

Hi @Goodiec.

Thanks for the PR. I tried going through the changes you made but looks like some of them are a result of formatting the code. Is that correct? If so, It would be best to always avoid committing formatting changes otherwise reviewing your code becomes difficult.

Hello @nibble0101, thank you for the review. Please could you elaborate more on this so I can make the corrections?

@nibble0101
Copy link
Contributor

@Goodiec What i mean is that there seems to be some changes in your commit which are not related to the issue you are trying to fix though I could be wrong. From the top right corner, you will notice github is indicating you have changed 58 lines of code. You removed 23 lines and added 35 lines. Are all those changes fixing issue #1357 or some of them are as a result of formatting the code?

@Goodiec
Copy link
Contributor Author

Goodiec commented Apr 13, 2021

@Goodiec What i mean is that there seems to be some changes in your commit which are not related to the issue you are trying to fix though I could be wrong. From the top right corner, you will notice github is indicating you have changed 58 lines of code. You removed 23 lines and added 35 lines. Are all those changes fixing issue #1357 or some of them are as a result of formatting the code?

OK @nibble0101, I think it is adding the whitespace changes that my editor is making. I will look at how to stop that from happening, thank you!

@avsm avsm added the pushed-to-staging pushed to staging.ocaml.org label Apr 19, 2021
@avsm avsm merged commit e9249df into ocaml:master Apr 21, 2021
@avsm
Copy link
Member

avsm commented Apr 21, 2021

Merged to the live site. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pushed-to-staging pushed to staging.ocaml.org
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with focus on clickable image
3 participants