-
Notifications
You must be signed in to change notification settings - Fork 86
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
Initial implementation of KImg component #404
Merged
thanksameeelian
merged 2 commits into
learningequality:develop
from
thanksameeelian:create-kimg-component
Jan 20, 2023
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
<template> | ||
|
||
<DocsPageTemplate apiDocs> | ||
<DocsPageSection title="Overview" anchor="#overview"> | ||
<div> | ||
The <DocsLibraryLink component="KImg" /> component displays images based on an image source and optional image | ||
dimensions provided by the implementer. | ||
|
||
<h3>Sample implementations of <DocsLibraryLink component="KImg" /> including props:</h3> | ||
|
||
<div> | ||
Dimensions may be either numbers or strings consisting of a numeral and valid units (e.g. <code>px</code>, | ||
<code>em</code>, <code>vh</code>). | ||
</div> | ||
|
||
<div class="img-example-1"> | ||
<DocsShow> | ||
<KImg | ||
:src="require('../assets/img_sample_for_kimg.png')" | ||
altText="Computers are run by bees" | ||
:height="'250.2px'" | ||
:width="225.5" | ||
:maxHeight="600" | ||
:minWidth="25" | ||
/> | ||
</DocsShow> | ||
<DocsShowCode language="html"> | ||
<KImg | ||
:src="require('../assets/img_sample_for_kimg.png')" | ||
altText="Computers are run by bees" | ||
:height="'250.2px'" | ||
:width="225.5" | ||
/> | ||
</DocsShowCode> | ||
</div> | ||
</div> | ||
|
||
<div class="img-example-2"> | ||
<DocsLibraryLink component="KImg" /> requires alternative text that describes the image unless | ||
<code>isDecorative</code> is <code>true</code>. In that case, any alt text provided will be overwritten to an | ||
empty string. | ||
<div> | ||
<div> | ||
<DocsShow> | ||
<KImg | ||
:src="require('../assets/img_sample_for_kimg.png')" | ||
isDecorative | ||
:height="50" | ||
:width="50" | ||
/> | ||
</DocsShow> | ||
</div> | ||
<DocsShowCode language="html"> | ||
<KImg | ||
:src="require('../assets/img_sample_for_kimg.png')" | ||
isDecorative | ||
:height="50" | ||
:width="50" | ||
/> | ||
</DocsShowCode> | ||
</div> | ||
</div> | ||
|
||
<div> | ||
If dimensions for the image are not specified, the size will default to the height and width of the source | ||
image. | ||
<DocsShow> | ||
<KImg | ||
:src="require('../assets/img_sample_for_kimg.png')" | ||
altText="Computers are run by bees" | ||
/> | ||
</DocsShow> | ||
<DocsShowCode language="html"> | ||
<KImg | ||
:src="require('../assets/img_sample_for_kimg.png')" | ||
altText="Computers are run by bees" | ||
/> | ||
</DocsShowCode> | ||
|
||
</div> | ||
</DocsPageSection> | ||
</DocsPageTemplate> | ||
|
||
</template> | ||
|
||
|
||
<script> | ||
|
||
export default {}; | ||
|
||
</script> | ||
|
||
|
||
<style lang="scss" scoped> | ||
|
||
.img-example-1 { | ||
display: flex; | ||
margin-top: 20px; | ||
} | ||
|
||
.img-example-2 { | ||
margin-top: 20px; | ||
margin-bottom: 20px; | ||
} | ||
|
||
.img-example-2 > div { | ||
display: flex; | ||
} | ||
|
||
.img-example-2 > div > div { | ||
margin-top: 30px; | ||
} | ||
|
||
</style> | ||
|
||
|
||
|
||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
<template> | ||
|
||
<div> | ||
<img | ||
:src="src" | ||
:alt="alternateText" | ||
:style="styleObject" | ||
> | ||
<slot></slot> | ||
</div> | ||
|
||
</template> | ||
|
||
|
||
<script> | ||
|
||
export default { | ||
name: 'KImg', | ||
props: { | ||
/** | ||
* Sets the image path | ||
*/ | ||
src: { | ||
type: String, | ||
default: null, | ||
}, | ||
/** | ||
* Alternate text for the image. This is required and will throw a warning when it's not provided (empty) unless isDecorative is true | ||
*/ | ||
altText: { | ||
type: String, | ||
default: '', | ||
}, | ||
/** | ||
* Sets the image as decorative. This sets the alt image property to an empty string. | ||
*/ | ||
isDecorative: { | ||
type: Boolean, | ||
default: false, | ||
}, | ||
/** | ||
* Sets the height for the component | ||
*/ | ||
height: { | ||
type: [Number, String], | ||
default: undefined, | ||
}, | ||
/** | ||
* Sets the width for the component | ||
*/ | ||
width: { | ||
type: [Number, String], | ||
default: undefined, | ||
}, | ||
/** | ||
* Sets the maximum height for the component | ||
*/ | ||
maxHeight: { | ||
type: [Number, String], | ||
default: undefined, | ||
}, | ||
/** | ||
* Sets the minimum height for the component | ||
*/ | ||
minHeight: { | ||
type: [Number, String], | ||
default: undefined, | ||
}, | ||
/** | ||
* Sets the maximum width for the component | ||
*/ | ||
maxWidth: { | ||
type: [Number, String], | ||
default: undefined, | ||
}, | ||
/** | ||
* Sets the minimum width for the component | ||
*/ | ||
minWidth: { | ||
type: [Number, String], | ||
default: undefined, | ||
}, | ||
}, | ||
data() { | ||
return { | ||
styleObject: { | ||
height: this.imgHeight, | ||
width: this.imgWidth, | ||
maxHeight: this.imgMaxHeight, | ||
minHeight: this.imgMinHeight, | ||
maxWidth: this.imgMaxWidth, | ||
minWidth: this.imgMinWidth, | ||
}, | ||
}; | ||
}, | ||
computed: { | ||
alternateText() { | ||
return this.isDecorative ? '' : this.altText; | ||
}, | ||
imgHeight() { | ||
return this.validateAndFormatUnits(this.height); | ||
}, | ||
imgWidth() { | ||
return this.validateAndFormatUnits(this.width); | ||
}, | ||
imgMaxHeight() { | ||
return this.validateAndFormatUnits(this.maxHeight); | ||
}, | ||
imgMinHeight() { | ||
return this.validateAndFormatUnits(this.minHeight); | ||
}, | ||
imgMaxWidth() { | ||
return this.validateAndFormatUnits(this.maxWidth); | ||
}, | ||
imgMinWidth() { | ||
return this.validateAndFormatUnits(this.minWidth); | ||
}, | ||
}, | ||
created() { | ||
if (!this.isDecorative && !this.altText) { | ||
throw new Error('Missing required prop - provide altText or indicate isDecorative'); | ||
} | ||
}, | ||
methods: { | ||
validateAndFormatUnits(propValue) { | ||
if (propValue) { | ||
if (!isNaN(propValue)) { | ||
return `${propValue}px`; | ||
} else { | ||
// split numbers apart from units | ||
const [, ...arr] = propValue.match(/(\d*\.?\d+)([a-zA-Z | %]*)/); | ||
const validUnits = [ | ||
'%', | ||
'cm', | ||
'em', | ||
'ex', | ||
'ch', | ||
'in', | ||
'lh', | ||
'mm', | ||
'px', | ||
'rem', | ||
'rlh', | ||
'vh', | ||
'vw', | ||
]; | ||
|
||
// if made up of valid numbers and valid units | ||
if (!isNaN(arr[0]) && validUnits.includes(arr[1])) { | ||
return propValue; | ||
} | ||
} | ||
} | ||
}, | ||
}, | ||
}; | ||
|
||
</script> | ||
|
||
|
||
<style scoped></style> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add the
keywords
property to allow for better searching within the documentation. For example, currently when you try to search an image component using the "image" keyword, the search would yield no results. Please see usages within this file for implementation detailsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't automatically showing up as outdated since i added code on the line below, but after checking the code please let me know whether you think the additions in my most recent commit are sufficient or you'd like to see additional keywords.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes made should suffice. Thanks @thanksameeelian. Non-blocking but would be good to follow existing patterns i.e. pass variable to keywords as opposed to the actual array object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! i checked out that pattern when i was adding the keywords here, but only saw the pattern of using a variable when multiple items had the same keywords, so the variable was being used in at least two places.
i don't think that pattern is applicable here, as
KImg
is the only item using those specific keywords. i am following the pattern i saw for each item that had a unique list of variables, as in those cases the array itself was passed. you can see this patten inKSelect
andKSwitch
in the table of contents. i think this makes sense because it avoids unnecessary abstraction.but if another item were to share the same keywords at kimg, i would totally agree that a variable would be the right call!