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.

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

Introduce BS version, move templates to a subfolder by version #1485

Merged
merged 3 commits into from
Feb 12, 2021
Merged

Introduce BS version, move templates to a subfolder by version #1485

merged 3 commits into from
Feb 12, 2021

Conversation

maelle
Copy link
Collaborator

@maelle maelle commented Feb 8, 2021

Same as #1481 but from this repo to get a preview.

@maelle
Copy link
Collaborator Author

maelle commented Feb 8, 2021

Ultimately I'd still like to add a BS4 folder before merging, that'd be exactly the same as the BS3 folder. This way when I make a PR editing the BS4 templates, it'll be clear how they differ from the BS3 templates. Is there a better way?

maelle
maelle commented Feb 8, 2021
View reviewed changes
R/render.r Outdated Show resolved Hide resolved
@maelle maelle mentioned this pull request Feb 8, 2021
Closed
hadley
hadley reviewed Feb 11, 2021
View reviewed changes
R/render.r Outdated Show resolved Hide resolved
R/render.r Outdated Show resolved Hide resolved
@maelle
move templates
070e5b8 
line

better error message

maybe better

fix

not x

fix ws
@maelle
Copy link
Collaborator Author

maelle commented Feb 12, 2021

@hadley What's your take on

Ultimately I'd still like to add a BS4 folder before merging, that'd be exactly the same as the BS3 folder. This way when I make a PR editing the BS4 templates, it'll be clear how they differ from the BS3 templates. Is there a better way?

Before adding the BS4 templates as a copy of BS3 templates, #1487 could be merged (it'll be easier to document those if they exist for both Bootstrap versions, and it's an easy enough change?)

hadley
hadley reviewed Feb 12, 2021
View reviewed changes
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

I still think it makes sense to merge this, and then do another PR that adds the initial BS4 templates.

R/render.r Outdated Show resolved Hide resolved
R/render.r Outdated Show resolved Hide resolved
R/render.r Outdated Show resolved Hide resolved
@maelle
Copy link
Collaborator Author

maelle commented Feb 12, 2021

I still think it makes sense to merge this, and then do another PR that adds the initial BS4 templates.

Ok I'll do that after this PR, and #1487 are merged

@maelle maelle changed the title move templates Introduce BS version, move templates to a subfolder by version Feb 12, 2021
@maelle maelle merged commit f58b8d2 into master Feb 12, 2021
@maelle maelle deleted the movetemplates branch February 12, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hadley hadley hadley approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@maelle @hadley