8000 Move primer to yarn by dometto · Pull Request #1937 · gollum/gollum · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Move primer to yarn #1937

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Jan 18, 2023
Merged

Move primer to yarn #1937

merged 19 commits into from
Jan 18, 2023

Conversation

dometto
Copy link
Member
@dometto dometto commented Jan 12, 2023 •
edited
Loading

Take 2. :)

It tuns out the 'product' component is necessary to style things like tables in the markdown body. However, this does not work neatly unless we import the primer components after importing our own template.scss. In turn, this caused primer to override some of our own styling from template.scss. I have addressed this by identifying which parts of template.scss are superfluous, which need to be included before primer, and which need to be included after primer.

  • Move our own .markdown_body styling to a separate file
  • The spinner with 'When was this page last modified?' is no longer there
  • Rebuild assets

@dometto dometto mentioned this pull request Jan 12, 2023
Draft
12 tasks
@dometto
Copy link
Member Author
dometto commented Jan 12, 2023

@benjaminwil @bartkamphorst I believe this solves all the styling issues @bartkamphorst discovered after having (prematurely) merged #1914. The problem was that some of our own styling from template.scss was being overridden by primer. Annoyingly, however, importing template.scss after primer also didn't work (presumably because some primer definitions were being used in template). So in this PR, I have:

  1. Radically cut some styling from template.scss that it looks like primer is already providing. I have tested as best as I could, but it would be great if you could give this branch a spin and see if you see anything strange.
  2. Moved some of the styling from template.scss that we don't want primer to override into app.scss, after primer is included.

If this approach seems sane, I'll move the new styling in app.scss to a separate new file (something like wiki_content.scss).

Fix jruby tests
abf277c 
Use nokogiri instead of regex in flaky test
@bartkamphorst
Copy link
Member
bartkamphorst commented Jan 14, 2023 •
edited
Loading

Overall it seems to look okay. Two things I noticed straight away:

  1. The styling of the tables no longer has the rounded corners from Add rounded corners to tables. Resolves #1693. #1881 .
    Screenshot 2023-01-14 at 17 40 39

EDIT: This seems to be straightforwardly caused by the table styling being removed from template.scss.erb.

  1. The spinner with 'When was this page last modified?' is no longer there.

@dometto
Copy link
Member Author
dometto commented Jan 16, 2023

This seems to be straightforwardly caused by the table styling being removed from template.scss.erb.

Thanks for catching! Re-added that styling.

The spinner with 'When was this page last modified?' is no longer there.

Again thanks for catching. Not sure yet why.

I've added the spinner issue to the todo for this PR. Apart from that issue, I think this is good to go. We may of course still discover slight differences in the styling in the future, but if they need to be addressed at all, it should be straightforward to fix them by looking at the changes in this PR.

@bartkamphorst
Copy link
Member
bartkamphorst commented Jan 17, 2023 •
edited
Loading

The problem with the spinner can be straightforwardly resolved by adding back the //= require spinner directive in app.scss. 😄

(Of course, that only restores the dotted-spinner but does not yet replace it with Primer's spinner component (see #1925).)

@dometto
Copy link
Member Author
dometto commented Jan 18, 2023

Ok, I'm merging this. This was more complicated than I hoped, and as I said there may be tiny style issues that I've missed. But I think it should be much easier to fix such issues now (also after #1915). Thanks for the feedback @benjaminwil and @bartkamphorst!

@dometto dometto merged commit 4208dbb into gollum:master Jan 18, 2023
@dometto dometto deleted the primer_yarn branch January 18, 2023 12:54
@benjaminwil benjaminwil mentioned this pull request Mar 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartkamphorst bartkamphorst bartkamphorst left review comments

@benjaminwil benjaminwil benjaminwil left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dometto @bartkamphorst @benjaminwil
0