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

Issue1052 subheadings with text #1055

Merged
merged 19 commits into from
Sep 14, 2020
Merged

Conversation

joelit
Copy link
Contributor

@joelit joelit commented Sep 4, 2020

The new layout to get rid of tables when displaying subheadings. The new heading styles should use this layout as its basis.

@joelit joelit requested a review from kouralex September 4, 2020 09:16
@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

Merging #1055 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1055      +/-   ##
============================================
+ Coverage     60.31%   60.32%   +0.01%     
- Complexity     1591     1592       +1     
============================================
  Files            32       32              
  Lines          4427     4429       +2     
============================================
+ Hits           2670     2672       +2     
  Misses         1757     1757              
Impacted Files Coverage Δ Complexity Δ
model/ConceptProperty.php 96.42% <100.00%> (+0.13%) 20.00 <1.00> (+1.00)

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 5fb7bf7...4f5d101. Read the comment docs.

@joelit joelit added this to the 2.8 milestone Sep 4, 2020
@joelit joelit marked this pull request as ready for review September 4, 2020 11:13
Copy link
Contributor

@kouralex kouralex left a comment

Choose a reason for hiding this comment

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

Whilst this PR gets rid of the table structure found on both ontology page and concept page, I encountered a few issues before this can be merged:

  1. The content language selection is on the left side now. See attached image below where image on the left hand side has been rendered using the code from this PR branch and the right hand side using the current master.
    image
  1. As one can see from the above image, this PR creates more space between the separators. Maybe a little bit less would suffice?
  2. The fonts on the ontology page for some are not alright (font + size, maybe other properties too). See below image.
    image
  3. I find it a little bit hard to follow the CSS declarations but e.g., setting negative values for the bottom property is something I would suggest to avoid as these things should now be easier to adjust with flex properties.

Otherwise, this PR seems to get rid of some tables which is good progress!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@joelit joelit requested a review from kouralex September 9, 2020 08:54
@joelit
Copy link
Contributor Author

joelit commented Sep 9, 2020

Whilst this PR gets rid of the table structure found on both ontology page and concept page, I encountered a few issues before this can be merged:

  1. The content language selection is on the left side now. See attached image below where image on the left hand side has been rendered using the code from this PR branch and the right hand side using the current master.19-f094e6e49b4e.png)

There was a conflict between bootstrap and the Skosmos css. I resolved it for now, though I suspect more effort should be made ensuring the class names won't conflict in the future.

  1. As one can see from the above image, this PR creates more space between the separators. Maybe a little bit less would suffice?

This was the closest replication of the previous layout. There is an extra pixel row underneath the row separator bar, but it looks good to me.

  1. The fonts on the ontology page for some are not alright (font + size, maybe other properties too).

Fixed.

  1. I find it a little bit hard to follow the CSS declarations but e.g., setting negative values for the bottom property is something I would suggest to avoid as these things should now be easier to adjust with flex properties.

It's true. The CSS declarations are hard to follow. Restructuring would be in order.

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.

2 participants