Update default bootstrap version to 4 (latest cdn) #746

ChangJoo-Park · 2018-04-11T15:04:42Z

Description of the Change

Alternate Designs

Benefits

Using latest version of Bootstrap 4

Possible Drawbacks

I tested on initialize template and after scaffolding resources. amber generate scaffold post

faustinoaq

Hi @ChangJoo-Park Thank you for your collaboration!, very appreciated! 😄

I left some comments 👍

Btw, I think we should update other generated views as well (forms, grids, show page, recipes, etc)

Ref: https://github.com/amberframework/amber/tree/master/src/amber/cli/templates

src/amber/cli/templates/app/src/views/layouts/application.slang.ecr

src/amber/cli/templates/app/src/views/home/index.slang.ecr

faustinoaq · 2018-04-11T15:31:31Z

src/amber/cli/templates/app/src/views/home/index.ecr.ecr

-    <a class="list-group-item list-group-item-action" target="_blank" href="https://github.com/veelenga/awesome-crystal">List of Awesome Crystal projects and shards</a>
-    <a class="list-group-item list-group-item-action" target="_blank" href="https://crystalshards.xyz">What's hot in Crystal right now</a>
+<div class="row">
+  <div id="logo" class="col-sm-6"></div>


:ditto: about the col-sm-6 class

I'll do fix indents

faustinoaq · 2018-04-11T16:09:19Z

Also auth links aren't right aligned: (in latest amber version v0.7.2 auth links are right aligned)

faustinoaq · 2018-04-11T16:13:20Z

I think we should add some top margin to the flash message box 👇

ChangJoo-Park · 2018-04-12T00:29:46Z

Thank you for your awesome reviews! I'll check these.

Can you explain for local development? Yesterday, I changed codes and pushed, created new repo again and again.

I think it is wrong approach. 😭

faustinoaq · 2018-04-12T01:03:14Z

Can you explain for local development? Yesterday, I changed codes and pushed, created new repo again and again.

@ChangJoo-Park Sure, no problem 😉

Be sure you have your ssh-keys well configured, see: GitHub guide
Fork this repo (you already forked this 👍 )
Clone your fork in your machine:

git clone [email protected]:ChangJoo-Park/amber.git
cd amber
git checkout bootstrap-4  # You already created this branch

Do the code changes you want and then:

git add . # add your modified files
git commit -m "Apply new changes" # commit changes
git push -a origin  # Upload all your changes

Finally you can see your commits here in this PR 😉

If you want to make new changes just repeat the step 4 the times you want 😄

BTW, if you can't do git push -a origin try to do git pull first. This happens because sometimes we update the PRs with our master branch.

faustinoaq · 2018-04-12T01:14:54Z

BTW, just to be clear, I think grids and forms need to be updated as well 😉

Also we should check views generated by:

amber generate scaffold Tasks name:string description:text done:bool
amber generate controller mypage
amber generate auth
amber generate error

And any other generator, (maybe recipes as well @damianham 👀 😅 )

ChangJoo-Park · 2018-04-12T05:04:16Z

Thanks @faustinoaq nice detailed explain.
I am going to fix today. :)

damianham · 2018-04-12T05:32:28Z

As per this issue
#724

The advice is to create a pull request with a single commit and a detailed commit log message so using -m would not allow you to create a detailed commit log message. Also you need to specify the new branchname in the push e.g.

I use

git commit # commit changes
git push -u origin bootstrap-4 # Upload all your changes

damianham · 2018-04-12T09:07:42Z

How can we get sign in /out and profile to pull right in the nav bar ?

ChangJoo-Park · 2018-04-12T10:00:57Z

@damianham Did you think like this?

<nav class="navbar navbar-expand-lg navbar-light bg-light">
  <a class="navbar-brand" href="#">Navbar w/ text</a>
  <button class="navbar-toggler" type="button" data-toggle="collapse" data-target="#navbarText" aria-controls="navbarText" aria-expanded="false" aria-label="Toggle navigation">
    <span class="navbar-toggler-icon"></span>
  </button>
  <div class="collapse navbar-collapse" id="navbarText">
    <ul class="navbar-nav mr-auto">
      <li class="nav-item active">
        <a class="nav-link" href="#">Home <span class="sr-only">(current)</span></a>
      </li>
      <li class="nav-item">
        <a class="nav-link" href="#">Features</a>
      </li>
      <li class="nav-item">
        <a class="nav-link" href="#">Pricing</a>
      </li>
    </ul>
    <span class="navbar-text">
      Navbar text with an inline element
    </span>
  </div>
</nav>

eliasjpr

Thank you for taking the time to contribute, we truly appreciate it. I have left some small comments

src/amber/cli/templates/app/src/views/home/index.slang.ecr

…right side of navbar

faustinoaq · 2018-04-17T17:11:23Z

is this intentional?

@faraazahmad Right now we're using links, I think we should consider using local assets for offline development, see #746 (comment)

@amberframework/contributors Is bad idea to store a local version of Bootstrap or jQuery? WDYT?

drujensen · 2018-04-17T17:17:16Z

@faustinoaq for simplicity, I recommend using the CDN. If you require offline, you can use an npm package.

https://www.npmjs.com/package/bootstrap

faustinoaq · 2018-04-17T17:59:46Z

@drujensen Oh, thank you, nice idea 👍

robacarp · 2018-04-18T14:14:23Z

The bootstrap website recommends using a little more verbose of a script and link tag:

<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0/css/bootstrap.min.css" integrity="sha384-Gn5384xqQ1aoWXA+058RXPxPg6fy4IWvTNh0E263XmFcJlSAwiGgFAW/dAiS6JXm" crossorigin="anonymous">
<script src="https://code.jquery.com/jquery-3.2.1.slim.min.js" integrity="sha384-KJ3o2DKtIkvYIK3UENzmM7KCkRr/rE9/Qpg6aAZGJwFDMVNA/GpGFF93hXpG5KkN" crossorigin="anonymous"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.12.9/umd/popper.min.js" integrity="sha384-ApNbgh9B+Y1QKtv3Rn7W3mgPxhU9K/ScQsAP7hUibX39j7fakFPskvXusvfa0b4Q" crossorigin="anonymous"></script>
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0/js/bootstrap.min.js" integrity="sha384-JZR6Spejh4U02d8jOt6vLEHfe/JQGiRRSQQxSfFWpi1MquVdAyjUar5+76PVCmYl" crossorigin="anonymous"></script>

Should we default to including the integrity and crossorigin options?

From the MDN on Subresource Integrity:

Using Content Delivery Networks (CDNs) to host files such as scripts and stylesheets that are shared among multiple sites can improve site performance and conserve bandwidth. However, using CDNs also comes with a risk, in that if an attacker gains control of a CDN, the attacker can inject arbitrary malicious content into files on the CDN (or replace the files completely) and thus can also potentially attack all sites that fetch files from that CDN.

faustinoaq · 2018-04-18T15:49:36Z

@ChangJoo-Park I just updated this PR with master branch.

If you get an error on git push try git pull first 😅

ChangJoo-Park · 2018-04-20T16:01:45Z

@faraazahmad I did not get any errors 😀

robacarp · 2018-04-25T23:14:26Z

@ChangJoo-Park any thoughts on adding the html attributes for subresources integrity as I mentioned above?

faustinoaq · 2018-05-12T23:14:24Z

@robacarp Yeah, I think html attributes is a nice idea to improve amber security in generated templates.

However, Do you know if these html attributes have some usability issue?

I mean, do html attributes get outdated or something like that?

otherwise, I think this PR is ready to merge, @ChangJoo-Park WDYT?

faustinoaq

I think this is ready to merge, we can open a new PR for other features/issues related to this

ChangJoo-Park · 2018-05-14T01:25:09Z

I am so sorry to reply late. I saw integrity and crossorigin attrbute. I think it may be good for security, but it is not must needed.

faustinoaq · 2018-05-14T01:47:09Z

I am so sorry to reply late. I saw integrity and crossorigin attrbute. I think it may be good for security, but it is not must needed.

@ChangJoo-Park Yeah, if this is required, then we can implement this in a new PR 😅

eliasjpr

I think this can be merged

ChangJoo-Park · 2018-05-15T13:56:16Z

Thanks everyone :)

ChangJoo-Park added 4 commits April 11, 2018 23:22

Update bootstrap 3 to 4

c90eeac

Update bootstrap 3 to 4 for ecr

c9071e8

Update for bootstrap 4, add row class div

9cc98cd

Add col-sm for bootstrap 4 row

f13dd97

faustinoaq added the pr:needs-review label Apr 11, 2018

faustinoaq requested a review from a team April 11, 2018 15:18

faustinoaq suggested changes Apr 11, 2018

View reviewed changes

ChangJoo-Park added 9 commits April 12, 2018 20:36

amberframework#746 fix indentations

45aeb1e

amberframework#746 Remove col-md-6 from index.slang.ecr

06e7ff1

amberframework#746 change jquery-slim to jquery

0e78956

amberframework#746 migrate pull-right to ml-auto for auth links

95457c5

amberframework#746 btn-xs to btn-sm. btn-xs is removed

3757408

amberframework#746 btn-default has removed. change to btn-light

e98e61f

amberframework#746 update checkbox for bootstrap 4

156ea70

amberframework#746 vertical-align: middle; for bootstrap 4 tables

4a2a5c3

amberframework#746 update alert classs for bootstrap 4

600a273

eliasjpr reviewed Apr 12, 2018

View reviewed changes

src/amber/cli/templates/app/src/views/home/index.slang.ecr Outdated Show resolved Hide resolved

src/amber/cli/templates/app/src/views/home/index.slang.ecr Outdated Show resolved Hide resolved

ChangJoo-Park added 2 commits April 12, 2018 22:36

amberframework#746 remove ml-auto, need to fix auth links are on the …

a426e1b

…right side of navbar

amberframework#746 using only class and id for div

0feda91

Merge branch 'master' into bootstrap-4

748b1b0

faraazahmad mentioned this pull request Apr 18, 2018

Add responsive design to default views #751

Closed

Merge branch 'master' into bootstrap-4

62a489e

faustinoaq added 2 commits April 30, 2018 12:50

Merge branch 'master' into bootstrap-4

962c176

Merge branch 'master' into bootstrap-4

67438aa

faustinoaq approved these changes May 13, 2018

View reviewed changes

robacarp approved these changes May 13, 2018

View reviewed changes

eliasjpr approved these changes May 14, 2018

View reviewed changes

robacarp and others added 2 commits May 14, 2018 15:19

Merge branch 'master' into bootstrap-4

8ca4070

Merge branch 'master' into bootstrap-4

c1bf89f

robacarp merged commit a557c82 into amberframework:master May 15, 2018

ChangJoo-Park deleted the bootstrap-4 branch May 15, 2018 13:56

faustinoaq removed the pr:needs-review label May 15, 2018

faustinoaq added kind:breaking-change kind:enhancement labels May 26, 2018

faustinoaq mentioned this pull request May 26, 2018

Update recipes to boostrap 4 amberframework/recipes#18

Closed

faustinoaq added the kind:dependency label Jun 20, 2018

faustinoaq added this to the Version 0.8.0 milestone Jun 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update default bootstrap version to 4 (latest cdn) #746

Update default bootstrap version to 4 (latest cdn) #746

ChangJoo-Park commented Apr 11, 2018

faustinoaq left a comment

faustinoaq Apr 11, 2018

ChangJoo-Park Apr 12, 2018

faustinoaq commented Apr 11, 2018

faustinoaq commented Apr 11, 2018 •

edited

Loading

ChangJoo-Park commented Apr 12, 2018 •

edited

Loading

faustinoaq commented Apr 12, 2018 •

edited

Loading

faustinoaq commented Apr 12, 2018

ChangJoo-Park commented Apr 12, 2018

damianham commented Apr 12, 2018 •

edited

Loading

damianham commented Apr 12, 2018

ChangJoo-Park commented Apr 12, 2018

eliasjpr left a comment

faustinoaq commented Apr 17, 2018

drujensen commented Apr 17, 2018

faustinoaq commented Apr 17, 2018

robacarp commented Apr 18, 2018

faustinoaq commented Apr 18, 2018

ChangJoo-Park commented Apr 20, 2018

robacarp commented Apr 25, 2018

faustinoaq commented May 12, 2018 •

edited

Loading

faustinoaq left a comment

ChangJoo-Park commented May 14, 2018

faustinoaq commented May 14, 2018 •

edited

Loading

eliasjpr left a comment

ChangJoo-Park commented May 15, 2018

Update default bootstrap version to 4 (latest cdn) #746

Update default bootstrap version to 4 (latest cdn) #746

Conversation

ChangJoo-Park commented Apr 11, 2018

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

faustinoaq left a comment

Choose a reason for hiding this comment

faustinoaq Apr 11, 2018

Choose a reason for hiding this comment

ChangJoo-Park Apr 12, 2018

Choose a reason for hiding this comment

faustinoaq commented Apr 11, 2018

faustinoaq commented Apr 11, 2018 • edited Loading

ChangJoo-Park commented Apr 12, 2018 • edited Loading

faustinoaq commented Apr 12, 2018 • edited Loading

faustinoaq commented Apr 12, 2018

ChangJoo-Park commented Apr 12, 2018

damianham commented Apr 12, 2018 • edited Loading

damianham commented Apr 12, 2018

ChangJoo-Park commented Apr 12, 2018

eliasjpr left a comment

Choose a reason for hiding this comment

faustinoaq commented Apr 17, 2018

drujensen commented Apr 17, 2018

faustinoaq commented Apr 17, 2018

robacarp commented Apr 18, 2018

faustinoaq commented Apr 18, 2018

ChangJoo-Park commented Apr 20, 2018

robacarp commented Apr 25, 2018

faustinoaq commented May 12, 2018 • edited Loading

faustinoaq left a comment

Choose a reason for hiding this comment

ChangJoo-Park commented May 14, 2018

faustinoaq commented May 14, 2018 • edited Loading

eliasjpr left a comment

Choose a reason for hiding this comment

ChangJoo-Park commented May 15, 2018

faustinoaq commented Apr 11, 2018 •

edited

Loading

ChangJoo-Park commented Apr 12, 2018 •

edited

Loading

faustinoaq commented Apr 12, 2018 •

edited

Loading

damianham commented Apr 12, 2018 •

edited

Loading

faustinoaq commented May 12, 2018 •

edited

Loading

faustinoaq commented May 14, 2018 •

edited

Loading