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

Added left/center/right alignment #21

Merged
merged 17 commits into from
Apr 19, 2023

Conversation

calumk
Copy link
Collaborator

@calumk calumk commented Apr 17, 2023

Added left/center/right justify block settings.

Attempted to mirror the style of your previous code for legibility.
uses align as the parameter, defaults to left

{
  type: 'alert',
  data: {
    type: 'danger',
    align: 'center',
    message:
      '<strong>Holy smokes!</strong><br/>Something seriously <em>bad</em> happened.',
  },
}

I used align icons from https://github.com/codex-team/icons
I opted to import svg's directly instead of importing the @codex-team/icons package, as you were already using svg in the asset folder - But this could be simplified in the future by adding @codex-team/icons as a dependancy

image

src/index.js Outdated
Comment on lines 18 to 20
import IconAlignLeft from '../assets/IconAlignLeft.svg';
import IconAlignCenter from '../assets/IconAlignCenter.svg';
import IconAlignRight from '../assets/IconAlignRight.svg';
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
import IconAlignLeft from '../assets/IconAlignLeft.svg';
import IconAlignCenter from '../assets/IconAlignCenter.svg';
import IconAlignRight from '../assets/IconAlignRight.svg';
import AlignLeftIcon from '../assets/align-left-icon.svg';
import AlignCenterIcon from '../assets/align-center-icon.svg';
import AlignRightIcon from '../assets/align-right-icon.svg';

Not a big deal, but it would help preserve the existing naming convention style.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If i commit here, without changing the file names themselves, i assume that will break things - not sure on the best way to resolve, commit then pull the repo then push a new change with filename changes and the code changes?

Copy link
Owner

Choose a reason for hiding this comment

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

Pull using git pull origin master, commit changes and then push it. Everything happens on your fork. Once the PR is merged, all the changes in your fork's master would then be merged to the master branch of this repo.

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
yarn.lock Outdated
resolved "https://registry.yarnpkg.com/@ampproject/remapping/-/remapping-2.2.0.tgz#56c133824780de3174aed5ab6834f3026790154d"
integrity sha512-qRmjj8nj9qmLTQXXmaR1cck3UXSRMPrbsLJAasZpF+t3riI71BXed5ebIOYwQntykeZuhjsdweEc9BxH5Jc26w==
"integrity" "sha512-qRmjj8nj9qmLTQXXmaR1cck3UXSRMPrbsLJAasZpF+t3riI71BXed5ebIOYwQntykeZuhjsdweEc9BxH5Jc26w=="
"resolved" "https://registry.npmjs.org/@ampproject/remapping/-/remapping-2.2.0.tgz"
Copy link
Owner

Choose a reason for hiding this comment

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

Not quite sure why this manifest is changed. Any idea (because we are not touching package.json at all in this PR)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I accidently used npm i / npm build instead of yarn while i was developing, possible i caused some issue here?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that's what seemed to have caused it. Can you please revert it?

@vishaltelangre
Copy link
Owner

@calumk Left some nits, but looks good otherwise!

calumk and others added 3 commits April 18, 2023 07:28
Co-authored-by: Vishal Telangre <vishaltelangre@gmail.com>
Co-authored-by: Vishal Telangre <vishaltelangre@gmail.com>
Co-authored-by: Vishal Telangre <vishaltelangre@gmail.com>
@calumk
Copy link
Collaborator Author

calumk commented Apr 18, 2023

@vishaltelangre updated notes

Sorry, this is one of the first times im commiting a PR requiring modification to somebody elses repo, so unsure on how to proceed on the icons issue

Do i make a new commit to my fork, and then open a new PR to resolve that? - what is the best way forward?

@vishaltelangre
Copy link
Owner

It appears that if you update your fork's master branch, it would automatically reflect in this PR.

src/index.scss Outdated Show resolved Hide resolved
src/index.scss Outdated Show resolved Hide resolved
calumk and others added 4 commits April 18, 2023 08:24
Co-authored-by: Vishal Telangre <vishaltelangre@gmail.com>
Co-authored-by: Vishal Telangre <vishaltelangre@gmail.com>
@calumk
Copy link
Collaborator Author

calumk commented Apr 18, 2023

I think I have now rolled back the yarn.lock - in the end i copied and pasted the whole file, because running yarn installseems to updates yarn.lock to newer versions

@vishaltelangre
Copy link
Owner

Thanks. 🙏 I will merge and publish a new version to npm registry tomorrow.

@vishaltelangre vishaltelangre merged commit a29504b into vishaltelangre:master Apr 19, 2023
vishaltelangre added a commit that referenced this pull request Apr 19, 2023
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