Skip to content

Change seamless panels to inherit any parent's colours - #2501

Merged
yiwen101 merged 24 commits into
MarkBind:masterfrom
jingting1412:change-seamless
Apr 17, 2024
Merged

Change seamless panels to inherit any parent's colours#2501
yiwen101 merged 24 commits into
MarkBind:masterfrom
jingting1412:change-seamless

Conversation

@jingting1412

@jingting1412jingting1412 commented Apr 5, 2024•
edited by kaixin-hc
Loading

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
#1697
Changed the seamless panels so that the background is transparent and it inherits the colours of any parents its contained in.

Anything you'd like to highlight/discuss:
As pointed out in the issue, this might break some edge cases (e.g. a seamless panel in a <box background-color="black" color="black"> box).

Testing instructions:
I've added some examples in the userguide to show the new look. markbind serve -d the docs to see the changes and try out different cases to see if it works or if there's any improvements.

Proposed commit message: (wrap lines at 72 characters)
Change seamless panels to inherit colours of any parents

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@codecov

codecovBot commented Apr 5, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.02%. Comparing base (fc776ba) to head (a721356).

❗ Current head a721356 differs from pull request most recent head b075167. Consider uploading reports for the commit b075167 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## master #2501 +/- ##
==========================================
+ Coverage 50.99% 51.02% +0.02% 
==========================================
Files 124 124 Lines 5383 5372 -11 Branches 1160 1159 -1 ==========================================
- Hits 2745 2741 -4 + Misses 2348 2341 -7 
Partials 290 290

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jingting1412

jingting1412 commented Apr 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Idk how my other allocate space for scrollbar commit got here 😞 but the changed files only reflect the changes relevant to this pr!

@kaixin-hc

kaixin-hc commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

Weird that the netlify deploy preview failed. Maybe it will fix itself on push

yucheng11122017
yucheng11122017 reviewed Apr 6, 2024
View reviewed changes

@yucheng11122017yucheng11122017 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @jingting1412 thanks for the work!
Functionality and all looks great - just some minor nits.

Could you also add some test cases for this behaviour? I think you should be able to add unit tests without too much issue

Comment threaddocs/userGuide/syntax/panels.md Outdated
</variable>
</include>

<box>

@yucheng11122017yucheng11122017Apr 6, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TBH i think this is too much documentation and examples.. I would rather just keep one, preferably the first because the other two are very blinding and I am old.

In fact... I'm a bit tempted to not have documentation for this because I feel like its too small and specific feature. What do the rest of the reviewers think?

@kaixin-hckaixin-hcApr 6, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just one is probably good enough! But the first one is not obvious enough - maybe play with the colours a bit so it stands out more.
Screenshot 2024-04-06 at 8 11 17 PM

I'm team documentation here, the use case becomes far clearer when you see how the seamless panel looks inside a colored background.

@jingting1412jingting1412Apr 7, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep I just left one example on the user guide so that user is at least aware of this property of seamless panels. Please tell me if the colour combo is not nice 😅

@kaixin-hckaixin-hcApr 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Colour looks good to me : )

Comment threadpackages/vue-components/src/panels/NestedPanel.vue
margin-top: 5px;
display: flex;
align-items: center;
color: inherit;

@yucheng11122017yucheng11122017Apr 6, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the difference between .morph-display-wrapper-seamless and .morph-display-wrapper only the inheritance of color?
In that case, could we make it such that .morph-display-wrapper-seamless is just color: inherit? So that if we decide to change like the margin-top or something, we only change it in one place
So the button will always have the class .morph-display-wrapper and will have the class .morph-display-wrapper-seamless if it is seamless

@jingting1412jingting1412Apr 7, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep sorry about that, I have made the changes accordingly

@jingting1412jingting1412Apr 12, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've changed it now so that both the seamless and non-seamless morph wrapper would share the same basic styles, and only morph-display-wrapper-seamless will have the color:inherit property. This allows for future changes to the styles of all morph wrappers to be done in one place and allows the seamless wrapper to have the same styles as the other wrappers

Comment threadpackages/vue-components/src/panels/NestedPanel.vue Outdated
Comment threadpackages/vue-components/src/panels/NestedPanel.vue Outdated
kaixin-hc
kaixin-hc reviewed Apr 6, 2024
View reviewed changes
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
kaixin-hc
kaixin-hc reviewed Apr 7, 2024
View reviewed changes
Comment threaddocs/userGuide/syntax/panels.md Outdated

:bulb: Seamless panels inherit the background colour and text colour of any parents!
<br/>
<panel type="seamless" header="This is an example seamless panel" popup-url="/userGuide/syntax/extra/loadContent.html">

@kaixin-hckaixin-hcApr 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why the popup-URL ?

@jingting1412jingting1412Apr 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm just making sure that all buttons follow the seamless behaviour when testing, I'll remove from the final example!

yucheng11122017
yucheng11122017 reviewed Apr 9, 2024
View reviewed changes

@yucheng11122017yucheng11122017 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small nit!

Comment threadpackages/vue-components/src/panels/NestedPanel.vue
<button :class="['morph-display-wrapper', 'btn', btnType]" @click="open()">
<button
:class="[isSeamless ? 'morph-display-wrapper-seamless': 'morph-display-wrapper',
'btn',

@yucheng11122017yucheng11122017Apr 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this button has the morph-display-wrapper anyway regardless of whether it is seamless or not?

@jingting1412jingting1412Apr 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I added the new morph-display-wrapper-seamless since I've noticed that the colour of the minimized panels would not inherit the parent's colour with just morph-display-wrapper

With just morph-display-wrapper:

Screen.Recording.2024-04-09.at.11.30.27.PM.mov

With morph-display-wrapper-seamless and morph-display-wrapper

Screen.Recording.2024-04-09.at.11.32.06.PM.mov

@yucheng11122017yucheng11122017Apr 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As in does the a seamless button also need the properties from morph-display-wrapper?

@jingting1412jingting1412Apr 12, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No seamless buttons wouldn't need properties from morph-display-wrapper, seamless buttons only appear when the panel is not minimised so it shouldn't need any properties from morph-display-wrapper

@kaixin-hckaixin-hcApr 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've taken a video for your reference that shows that toggling these properties would make a visual difference. The caret button seems to also be a seamless button which appears when the panel is minimised, so the non seamless panels and the seamless panels then have slightly different styling due to this line of code.

Screen.Recording.2024-04-12.at.7.54.19.PM.mov

@yucheng11122017yucheng11122017Apr 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rather than doing this or else statement, could all buttons have morph-display-wrapper by default and then only seamless buttons will have morph-display-wrapper-seamless?

@jingting1412jingting1412Apr 16, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yep sure I've implemented it as such now

yucheng11122017
yucheng11122017 approved these changes Apr 11, 2024
View reviewed changes

@yucheng11122017yucheng11122017 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM except for one small clarification!

yucheng11122017
yucheng11122017 approved these changes Apr 16, 2024
View reviewed changes

@yucheng11122017yucheng11122017 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@yiwen101
yiwen101 merged commit 8253f82 into MarkBind:masterApr 17, 2024
@yiwen101yiwen101 added the r.Patch Version resolver: increment by 0.0.1 label Apr 17, 2024
itsyme pushed a commit that referenced this pull request Apr 19, 2024
@jingting1412@yucheng11122017
@kaixin-hc@itsyme
Enable seamless panels to inherit any parent's colours (#2501)
8063cdb
* Change seamless panels
* Improve look of seamless panels
* Add tests
* Update userguide example
---------
Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
Co-authored-by: Hannah <59471320+kaixin-hc@users.noreply.github.com>
@damithc

damithc commented May 3, 2024

Copy link
Copy Markdown
Contributor

Feature in action https://nus-cs2103-ay2324s2.github.io/website/schedule/week8/topics.html

image

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaixin-hckaixin-hckaixin-hc left review comments
@yucheng11122017yucheng11122017yucheng11122017 approved these changes

Assignees

No one assigned

Labels

r.PatchVersion resolver: increment by 0.0.1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jingting1412@kaixin-hc@damithc@yucheng11122017@yiwen101
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Change seamless panels to inherit any parent's colours by jingting1412 · Pull Request #2501 · MarkBind/markbind · GitHub
Skip to content
Change seamless panels to inherit any parent's colours - #2501
Merged
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Apr 17, 2024
Merged
Change seamless panels to inherit any parent's colours#2501
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Conversation
@jingting1412
@jingting1412jingting1412
commented
Apr 5, 2024•

edited by kaixin-hc

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request?
  •  Documentation update
  •  Bug fix
  •  Feature addition or enhancement
  •  Code maintenance
  •  DevOps
  •  Improve developer experience
  •  Others, please explain:
Overview of changes:
#1697

Changed the seamless panels so that the background is transparent and it inherits the colours of any parents its contained in.
Anything you'd like to highlight/discuss:

As pointed out in the issue, this might break some edge cases (e.g. a seamless panel in a <box background-color="black" color="black"> box).
Testing instructions:

I've added some examples in the userguide to show the new look. markbind serve -d the docs to see the changes and try out different cases to see if it works or if there's any improvements.
Proposed commit message: (wrap lines at 72 characters)

Change seamless panels to inherit colours of any parents
Checklist: ☑️
  •  Updated the documentation for feature additions and enhancements
  •  Added tests for bug fixes or features
  •  Linked all related issues
  •  No unrelated changes 
Reviewer checklist:
Indicate the SEMVER impact of the PR:
  •  Major (when you make incompatible API changes)
  •  Minor (when you add functionality in a backward compatible manner)
  •  Patch (when you make backward compatible bug fixes)
At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.
Breaking change release note preparation (if applicable):
  • To be included in the release note for any feature that is made obsolete/breaking
Give a brief explanation note about:
  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).
@codecov
codecovBot
commented
Apr 5, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 51.02%. Comparing base (fc776ba) to head (a721356).
❗ Current head a721356 differs from pull request most recent head b075167. Consider uploading reports for the commit b075167 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## master #2501 +/- ##
==========================================
+ Coverage 50.99% 51.02% +0.02% 
==========================================
Files 124 124 Lines 5383 5372 -11 Branches 1160 1159 -1 ==========================================
- Hits 2745 2741 -4 + Misses 2348 2341 -7 
Partials 290 290 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@jingting1412
jingting1412
commented
Apr 5, 2024

Copy link


Copy Markdown

ContributorAuthor
Idk how my other allocate space for scrollbar commit got here 😞 but the changed files only reflect the changes relevant to this pr!
@kaixin-hc
kaixin-hc
commented
Apr 6, 2024

Copy link


Copy Markdown

Contributor
Weird that the netlify deploy preview failed. Maybe it will fix itself on push
yucheng11122017
yucheng11122017
reviewed
Apr 6, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Hi @jingting1412 thanks for the work!

Functionality and all looks great - just some minor nits.
Could you also add some test cases for this behaviour? I think you should be able to add unit tests without too much issue
Comment threaddocs/userGuide/syntax/panels.md
Outdated

</variable>
</include>

<box>
@yucheng11122017yucheng11122017Apr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

TBH i think this is too much documentation and examples.. I would rather just keep one, preferably the first because the other two are very blinding and I am old.
In fact... I'm a bit tempted to not have documentation for this because I feel like its too small and specific feature. What do the rest of the reviewers think?
@kaixin-hckaixin-hcApr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one is probably good enough! But the first one is not obvious enough - maybe play with the colours a bit so it stands out more.
Screenshot 2024-04-06 at 8 11 17 PM
I'm team documentation here, the use case becomes far clearer when you see how the seamless panel looks inside a colored background.
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep I just left one example on the user guide so that user is at least aware of this property of seamless panels. Please tell me if the colour combo is not nice 😅
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Colour looks good to me : )
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 margin-top: 5px;
 display: flex;
 align-items: center;
 color: inherit;
@yucheng11122017yucheng11122017Apr 6, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is the difference between .morph-display-wrapper-seamless and .morph-display-wrapper only the inheritance of color?

In that case, could we make it such that .morph-display-wrapper-seamless is just color: inherit? So that if we decide to change like the margin-top or something, we only change it in one place

So the button will always have the class .morph-display-wrapper and will have the class .morph-display-wrapper-seamless if it is seamless
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep sorry about that, I have made the changes accordingly
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I've changed it now so that both the seamless and non-seamless morph wrapper would share the same basic styles, and only morph-display-wrapper-seamless will have the color:inherit property. This allows for future changes to the styles of all morph wrappers to be done in one place and allows the seamless wrapper to have the same styles as the other wrappers
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

kaixin-hc
kaixin-hc
reviewed
Apr 6, 2024
View reviewed changes
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
kaixin-hc
kaixin-hc
reviewed
Apr 7, 2024
View reviewed changes
Comment threaddocs/userGuide/syntax/panels.md
Outdated


 :bulb: Seamless panels inherit the background colour and text colour of any parents!
 <br/>
 <panel type="seamless" header="This is an example seamless panel" popup-url="/userGuide/syntax/extra/loadContent.html">
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why the popup-URL ?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'm just making sure that all buttons follow the seamless behaviour when testing, I'll remove from the final example!
yucheng11122017
yucheng11122017
reviewed
Apr 9, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Small nit!
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 <button :class="['morph-display-wrapper', 'btn', btnType]" @click="open()">
 <button
 :class="[isSeamless ? 'morph-display-wrapper-seamless': 'morph-display-wrapper',
 'btn',
@yucheng11122017yucheng11122017Apr 9, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't this button has the morph-display-wrapper anyway regardless of whether it is seamless or not?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I added the new morph-display-wrapper-seamless since I've noticed that the colour of the minimized panels would not inherit the parent's colour with just morph-display-wrapper
With just morph-display-wrapper:
Screen.Recording.2024-04-09.at.11.30.27.PM.mov
With morph-display-wrapper-seamless and morph-display-wrapper
Screen.Recording.2024-04-09.at.11.32.06.PM.mov
@yucheng11122017yucheng11122017Apr 11, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

As in does the a seamless button also need the properties from morph-display-wrapper?
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No seamless buttons wouldn't need properties from morph-display-wrapper, seamless buttons only appear when the panel is not minimised so it shouldn't need any properties from morph-display-wrapper
@kaixin-hckaixin-hcApr 12, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I've taken a video for your reference that shows that toggling these properties would make a visual difference. The caret button seems to also be a seamless button which appears when the panel is minimised, so the non seamless panels and the seamless panels then have slightly different styling due to this line of code.
Screen.Recording.2024-04-12.at.7.54.19.PM.mov
@yucheng11122017yucheng11122017Apr 16, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Rather than doing this or else statement, could all buttons have morph-display-wrapper by default and then only seamless buttons will have morph-display-wrapper-seamless?
@jingting1412jingting1412Apr 16, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

yep sure I've implemented it as such now
yucheng11122017
yucheng11122017
approved these changes
Apr 11, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM except for one small clarification!
yucheng11122017
yucheng11122017
approved these changes
Apr 16, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@yiwen101
yiwen101
merged commit 8253f82
into
MarkBind:masterApr 17, 2024
@yiwen101yiwen101
added
the 
r.Patch
Version resolver: increment by 0.0.1
label
Apr 17, 2024
itsyme pushed a commit
that referenced
this pull request
Apr 19, 2024
@jingting1412@yucheng11122017
@kaixin-hc@itsyme
Enable seamless panels to inherit any parent's colours (#2501)
8063cdb
* Change seamless panels
* Improve look of seamless panels
* Add tests
* Update userguide example
---------
Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
Co-authored-by: Hannah <59471320+kaixin-hc@users.noreply.github.com>
@damithc
damithc
commented
May 3, 2024

Copy link


Copy Markdown

Contributor
Feature in action https://nus-cs2103-ay2324s2.github.io/website/schedule/week8/topics.html
image
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kaixin-hckaixin-hckaixin-hc left review comments
@yucheng11122017yucheng11122017yucheng11122017 approved these changes

Assignees


No one assigned


Labels

r.PatchVersion resolver: increment by 0.0.1

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jingting1412@kaixin-hc@damithc@yucheng11122017@yiwen101

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Change seamless panels to inherit any parent's colours by jingting1412 · Pull Request #2501 · MarkBind/markbind · GitHub
Skip to content
Change seamless panels to inherit any parent's colours - #2501
Merged
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Apr 17, 2024
Merged
Change seamless panels to inherit any parent's colours#2501
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Conversation
@jingting1412
@jingting1412jingting1412
commented
Apr 5, 2024•

edited by kaixin-hc

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request?
  •  Documentation update
  •  Bug fix
  •  Feature addition or enhancement
  •  Code maintenance
  •  DevOps
  •  Improve developer experience
  •  Others, please explain:
Overview of changes:
#1697

Changed the seamless panels so that the background is transparent and it inherits the colours of any parents its contained in.
Anything you'd like to highlight/discuss:

As pointed out in the issue, this might break some edge cases (e.g. a seamless panel in a <box background-color="black" color="black"> box).
Testing instructions:

I've added some examples in the userguide to show the new look. markbind serve -d the docs to see the changes and try out different cases to see if it works or if there's any improvements.
Proposed commit message: (wrap lines at 72 characters)

Change seamless panels to inherit colours of any parents
Checklist: ☑️
  •  Updated the documentation for feature additions and enhancements
  •  Added tests for bug fixes or features
  •  Linked all related issues
  •  No unrelated changes 
Reviewer checklist:
Indicate the SEMVER impact of the PR:
  •  Major (when you make incompatible API changes)
  •  Minor (when you add functionality in a backward compatible manner)
  •  Patch (when you make backward compatible bug fixes)
At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.
Breaking change release note preparation (if applicable):
  • To be included in the release note for any feature that is made obsolete/breaking
Give a brief explanation note about:
  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).
@codecov
codecovBot
commented
Apr 5, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 51.02%. Comparing base (fc776ba) to head (a721356).
❗ Current head a721356 differs from pull request most recent head b075167. Consider uploading reports for the commit b075167 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## master #2501 +/- ##
==========================================
+ Coverage 50.99% 51.02% +0.02% 
==========================================
Files 124 124 Lines 5383 5372 -11 Branches 1160 1159 -1 ==========================================
- Hits 2745 2741 -4 + Misses 2348 2341 -7 
Partials 290 290 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@jingting1412
jingting1412
commented
Apr 5, 2024

Copy link


Copy Markdown

ContributorAuthor
Idk how my other allocate space for scrollbar commit got here 😞 but the changed files only reflect the changes relevant to this pr!
@kaixin-hc
kaixin-hc
commented
Apr 6, 2024

Copy link


Copy Markdown

Contributor
Weird that the netlify deploy preview failed. Maybe it will fix itself on push
yucheng11122017
yucheng11122017
reviewed
Apr 6, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Hi @jingting1412 thanks for the work!

Functionality and all looks great - just some minor nits.
Could you also add some test cases for this behaviour? I think you should be able to add unit tests without too much issue
Comment threaddocs/userGuide/syntax/panels.md
Outdated

</variable>
</include>

<box>
@yucheng11122017yucheng11122017Apr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

TBH i think this is too much documentation and examples.. I would rather just keep one, preferably the first because the other two are very blinding and I am old.
In fact... I'm a bit tempted to not have documentation for this because I feel like its too small and specific feature. What do the rest of the reviewers think?
@kaixin-hckaixin-hcApr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one is probably good enough! But the first one is not obvious enough - maybe play with the colours a bit so it stands out more.
Screenshot 2024-04-06 at 8 11 17 PM
I'm team documentation here, the use case becomes far clearer when you see how the seamless panel looks inside a colored background.
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep I just left one example on the user guide so that user is at least aware of this property of seamless panels. Please tell me if the colour combo is not nice 😅
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Colour looks good to me : )
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 margin-top: 5px;
 display: flex;
 align-items: center;
 color: inherit;
@yucheng11122017yucheng11122017Apr 6, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is the difference between .morph-display-wrapper-seamless and .morph-display-wrapper only the inheritance of color?

In that case, could we make it such that .morph-display-wrapper-seamless is just color: inherit? So that if we decide to change like the margin-top or something, we only change it in one place

So the button will always have the class .morph-display-wrapper and will have the class .morph-display-wrapper-seamless if it is seamless
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep sorry about that, I have made the changes accordingly
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I've changed it now so that both the seamless and non-seamless morph wrapper would share the same basic styles, and only morph-display-wrapper-seamless will have the color:inherit property. This allows for future changes to the styles of all morph wrappers to be done in one place and allows the seamless wrapper to have the same styles as the other wrappers
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

kaixin-hc
kaixin-hc
reviewed
Apr 6, 2024
View reviewed changes
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
kaixin-hc
kaixin-hc
reviewed
Apr 7, 2024
View reviewed changes
Comment threaddocs/userGuide/syntax/panels.md
Outdated


 :bulb: Seamless panels inherit the background colour and text colour of any parents!
 <br/>
 <panel type="seamless" header="This is an example seamless panel" popup-url="/userGuide/syntax/extra/loadContent.html">
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why the popup-URL ?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'm just making sure that all buttons follow the seamless behaviour when testing, I'll remove from the final example!
yucheng11122017
yucheng11122017
reviewed
Apr 9, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Small nit!
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 <button :class="['morph-display-wrapper', 'btn', btnType]" @click="open()">
 <button
 :class="[isSeamless ? 'morph-display-wrapper-seamless': 'morph-display-wrapper',
 'btn',
@yucheng11122017yucheng11122017Apr 9, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't this button has the morph-display-wrapper anyway regardless of whether it is seamless or not?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I added the new morph-display-wrapper-seamless since I've noticed that the colour of the minimized panels would not inherit the parent's colour with just morph-display-wrapper
With just morph-display-wrapper:
Screen.Recording.2024-04-09.at.11.30.27.PM.mov
With morph-display-wrapper-seamless and morph-display-wrapper
Screen.Recording.2024-04-09.at.11.32.06.PM.mov
@yucheng11122017yucheng11122017Apr 11, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

As in does the a seamless button also need the properties from morph-display-wrapper?
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No seamless buttons wouldn't need properties from morph-display-wrapper, seamless buttons only appear when the panel is not minimised so it shouldn't need any properties from morph-display-wrapper
@kaixin-hckaixin-hcApr 12, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I've taken a video for your reference that shows that toggling these properties would make a visual difference. The caret button seems to also be a seamless button which appears when the panel is minimised, so the non seamless panels and the seamless panels then have slightly different styling due to this line of code.
Screen.Recording.2024-04-12.at.7.54.19.PM.mov
@yucheng11122017yucheng11122017Apr 16, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Rather than doing this or else statement, could all buttons have morph-display-wrapper by default and then only seamless buttons will have morph-display-wrapper-seamless?
@jingting1412jingting1412Apr 16, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

yep sure I've implemented it as such now
yucheng11122017
yucheng11122017
approved these changes
Apr 11, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM except for one small clarification!
yucheng11122017
yucheng11122017
approved these changes
Apr 16, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@yiwen101
yiwen101
merged commit 8253f82
into
MarkBind:masterApr 17, 2024
@yiwen101yiwen101
added
the 
r.Patch
Version resolver: increment by 0.0.1
label
Apr 17, 2024
itsyme pushed a commit
that referenced
this pull request
Apr 19, 2024
@jingting1412@yucheng11122017
@kaixin-hc@itsyme
Enable seamless panels to inherit any parent's colours (#2501)
8063cdb
* Change seamless panels
* Improve look of seamless panels
* Add tests
* Update userguide example
---------
Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
Co-authored-by: Hannah <59471320+kaixin-hc@users.noreply.github.com>
@damithc
damithc
commented
May 3, 2024

Copy link


Copy Markdown

Contributor
Feature in action https://nus-cs2103-ay2324s2.github.io/website/schedule/week8/topics.html
image
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kaixin-hckaixin-hckaixin-hc left review comments
@yucheng11122017yucheng11122017yucheng11122017 approved these changes

Assignees


No one assigned


Labels

r.PatchVersion resolver: increment by 0.0.1

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jingting1412@kaixin-hc@damithc@yucheng11122017@yiwen101

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Change seamless panels to inherit any parent's colours by jingting1412 · Pull Request #2501 · MarkBind/markbind · GitHub
Skip to content
Change seamless panels to inherit any parent's colours - #2501
Merged
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Apr 17, 2024
Merged
Change seamless panels to inherit any parent's colours#2501
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Conversation
@jingting1412
@jingting1412jingting1412
commented
Apr 5, 2024•

edited by kaixin-hc

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request?
  •  Documentation update
  •  Bug fix
  •  Feature addition or enhancement
  •  Code maintenance
  •  DevOps
  •  Improve developer experience
  •  Others, please explain:
Overview of changes:
#1697

Changed the seamless panels so that the background is transparent and it inherits the colours of any parents its contained in.
Anything you'd like to highlight/discuss:

As pointed out in the issue, this might break some edge cases (e.g. a seamless panel in a <box background-color="black" color="black"> box).
Testing instructions:

I've added some examples in the userguide to show the new look. markbind serve -d the docs to see the changes and try out different cases to see if it works or if there's any improvements.
Proposed commit message: (wrap lines at 72 characters)

Change seamless panels to inherit colours of any parents
Checklist: ☑️
  •  Updated the documentation for feature additions and enhancements
  •  Added tests for bug fixes or features
  •  Linked all related issues
  •  No unrelated changes 
Reviewer checklist:
Indicate the SEMVER impact of the PR:
  •  Major (when you make incompatible API changes)
  •  Minor (when you add functionality in a backward compatible manner)
  •  Patch (when you make backward compatible bug fixes)
At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.
Breaking change release note preparation (if applicable):
  • To be included in the release note for any feature that is made obsolete/breaking
Give a brief explanation note about:
  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).
@codecov
codecovBot
commented
Apr 5, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 51.02%. Comparing base (fc776ba) to head (a721356).
❗ Current head a721356 differs from pull request most recent head b075167. Consider uploading reports for the commit b075167 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## master #2501 +/- ##
==========================================
+ Coverage 50.99% 51.02% +0.02% 
==========================================
Files 124 124 Lines 5383 5372 -11 Branches 1160 1159 -1 ==========================================
- Hits 2745 2741 -4 + Misses 2348 2341 -7 
Partials 290 290 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@jingting1412
jingting1412
commented
Apr 5, 2024

Copy link


Copy Markdown

ContributorAuthor
Idk how my other allocate space for scrollbar commit got here 😞 but the changed files only reflect the changes relevant to this pr!
@kaixin-hc
kaixin-hc
commented
Apr 6, 2024

Copy link


Copy Markdown

Contributor
Weird that the netlify deploy preview failed. Maybe it will fix itself on push
yucheng11122017
yucheng11122017
reviewed
Apr 6, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Hi @jingting1412 thanks for the work!

Functionality and all looks great - just some minor nits.
Could you also add some test cases for this behaviour? I think you should be able to add unit tests without too much issue
Comment threaddocs/userGuide/syntax/panels.md
Outdated

</variable>
</include>

<box>
@yucheng11122017yucheng11122017Apr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

TBH i think this is too much documentation and examples.. I would rather just keep one, preferably the first because the other two are very blinding and I am old.
In fact... I'm a bit tempted to not have documentation for this because I feel like its too small and specific feature. What do the rest of the reviewers think?
@kaixin-hckaixin-hcApr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one is probably good enough! But the first one is not obvious enough - maybe play with the colours a bit so it stands out more.
Screenshot 2024-04-06 at 8 11 17 PM
I'm team documentation here, the use case becomes far clearer when you see how the seamless panel looks inside a colored background.
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep I just left one example on the user guide so that user is at least aware of this property of seamless panels. Please tell me if the colour combo is not nice 😅
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Colour looks good to me : )
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 margin-top: 5px;
 display: flex;
 align-items: center;
 color: inherit;
@yucheng11122017yucheng11122017Apr 6, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is the difference between .morph-display-wrapper-seamless and .morph-display-wrapper only the inheritance of color?

In that case, could we make it such that .morph-display-wrapper-seamless is just color: inherit? So that if we decide to change like the margin-top or something, we only change it in one place

So the button will always have the class .morph-display-wrapper and will have the class .morph-display-wrapper-seamless if it is seamless
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep sorry about that, I have made the changes accordingly
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I've changed it now so that both the seamless and non-seamless morph wrapper would share the same basic styles, and only morph-display-wrapper-seamless will have the color:inherit property. This allows for future changes to the styles of all morph wrappers to be done in one place and allows the seamless wrapper to have the same styles as the other wrappers
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

kaixin-hc
kaixin-hc
reviewed
Apr 6, 2024
View reviewed changes
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
kaixin-hc
kaixin-hc
reviewed
Apr 7, 2024
View reviewed changes
Comment threaddocs/userGuide/syntax/panels.md
Outdated


 :bulb: Seamless panels inherit the background colour and text colour of any parents!
 <br/>
 <panel type="seamless" header="This is an example seamless panel" popup-url="/userGuide/syntax/extra/loadContent.html">
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why the popup-URL ?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'm just making sure that all buttons follow the seamless behaviour when testing, I'll remove from the final example!
yucheng11122017
yucheng11122017
reviewed
Apr 9, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Small nit!
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 <button :class="['morph-display-wrapper', 'btn', btnType]" @click="open()">
 <button
 :class="[isSeamless ? 'morph-display-wrapper-seamless': 'morph-display-wrapper',
 'btn',
@yucheng11122017yucheng11122017Apr 9, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't this button has the morph-display-wrapper anyway regardless of whether it is seamless or not?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I added the new morph-display-wrapper-seamless since I've noticed that the colour of the minimized panels would not inherit the parent's colour with just morph-display-wrapper
With just morph-display-wrapper:
Screen.Recording.2024-04-09.at.11.30.27.PM.mov
With morph-display-wrapper-seamless and morph-display-wrapper
Screen.Recording.2024-04-09.at.11.32.06.PM.mov
@yucheng11122017yucheng11122017Apr 11, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

As in does the a seamless button also need the properties from morph-display-wrapper?
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No seamless buttons wouldn't need properties from morph-display-wrapper, seamless buttons only appear when the panel is not minimised so it shouldn't need any properties from morph-display-wrapper
@kaixin-hckaixin-hcApr 12, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I've taken a video for your reference that shows that toggling these properties would make a visual difference. The caret button seems to also be a seamless button which appears when the panel is minimised, so the non seamless panels and the seamless panels then have slightly different styling due to this line of code.
Screen.Recording.2024-04-12.at.7.54.19.PM.mov
@yucheng11122017yucheng11122017Apr 16, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Rather than doing this or else statement, could all buttons have morph-display-wrapper by default and then only seamless buttons will have morph-display-wrapper-seamless?
@jingting1412jingting1412Apr 16, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

yep sure I've implemented it as such now
yucheng11122017
yucheng11122017
approved these changes
Apr 11, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM except for one small clarification!
yucheng11122017
yucheng11122017
approved these changes
Apr 16, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@yiwen101
yiwen101
merged commit 8253f82
into
MarkBind:masterApr 17, 2024
@yiwen101yiwen101
added
the 
r.Patch
Version resolver: increment by 0.0.1
label
Apr 17, 2024
itsyme pushed a commit
that referenced
this pull request
Apr 19, 2024
@jingting1412@yucheng11122017
@kaixin-hc@itsyme
Enable seamless panels to inherit any parent's colours (#2501)
8063cdb
* Change seamless panels
* Improve look of seamless panels
* Add tests
* Update userguide example
---------
Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
Co-authored-by: Hannah <59471320+kaixin-hc@users.noreply.github.com>
@damithc
damithc
commented
May 3, 2024

Copy link


Copy Markdown

Contributor
Feature in action https://nus-cs2103-ay2324s2.github.io/website/schedule/week8/topics.html
image
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kaixin-hckaixin-hckaixin-hc left review comments
@yucheng11122017yucheng11122017yucheng11122017 approved these changes

Assignees


No one assigned


Labels

r.PatchVersion resolver: increment by 0.0.1

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jingting1412@kaixin-hc@damithc@yucheng11122017@yiwen101

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Change seamless panels to inherit any parent's colours by jingting1412 · Pull Request #2501 · MarkBind/markbind · GitHub
Skip to content
Change seamless panels to inherit any parent's colours - #2501
Merged
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Apr 17, 2024
Merged
Change seamless panels to inherit any parent's colours#2501
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Conversation
@jingting1412
@jingting1412jingting1412
commented
Apr 5, 2024•

edited by kaixin-hc

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request?
  •  Documentation update
  •  Bug fix
  •  Feature addition or enhancement
  •  Code maintenance
  •  DevOps
  •  Improve developer experience
  •  Others, please explain:
Overview of changes:
#1697

Changed the seamless panels so that the background is transparent and it inherits the colours of any parents its contained in.
Anything you'd like to highlight/discuss:

As pointed out in the issue, this might break some edge cases (e.g. a seamless panel in a <box background-color="black" color="black"> box).
Testing instructions:

I've added some examples in the userguide to show the new look. markbind serve -d the docs to see the changes and try out different cases to see if it works or if there's any improvements.
Proposed commit message: (wrap lines at 72 characters)

Change seamless panels to inherit colours of any parents
Checklist: ☑️
  •  Updated the documentation for feature additions and enhancements
  •  Added tests for bug fixes or features
  •  Linked all related issues
  •  No unrelated changes 
Reviewer checklist:
Indicate the SEMVER impact of the PR:
  •  Major (when you make incompatible API changes)
  •  Minor (when you add functionality in a backward compatible manner)
  •  Patch (when you make backward compatible bug fixes)
At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.
Breaking change release note preparation (if applicable):
  • To be included in the release note for any feature that is made obsolete/breaking
Give a brief explanation note about:
  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).
@codecov
codecovBot
commented
Apr 5, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 51.02%. Comparing base (fc776ba) to head (a721356).
❗ Current head a721356 differs from pull request most recent head b075167. Consider uploading reports for the commit b075167 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## master #2501 +/- ##
==========================================
+ Coverage 50.99% 51.02% +0.02% 
==========================================
Files 124 124 Lines 5383 5372 -11 Branches 1160 1159 -1 ==========================================
- Hits 2745 2741 -4 + Misses 2348 2341 -7 
Partials 290 290 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@jingting1412
jingting1412
commented
Apr 5, 2024

Copy link


Copy Markdown

ContributorAuthor
Idk how my other allocate space for scrollbar commit got here 😞 but the changed files only reflect the changes relevant to this pr!
@kaixin-hc
kaixin-hc
commented
Apr 6, 2024

Copy link


Copy Markdown

Contributor
Weird that the netlify deploy preview failed. Maybe it will fix itself on push
yucheng11122017
yucheng11122017
reviewed
Apr 6, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Hi @jingting1412 thanks for the work!

Functionality and all looks great - just some minor nits.
Could you also add some test cases for this behaviour? I think you should be able to add unit tests without too much issue
Comment threaddocs/userGuide/syntax/panels.md
Outdated

</variable>
</include>

<box>
@yucheng11122017yucheng11122017Apr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

TBH i think this is too much documentation and examples.. I would rather just keep one, preferably the first because the other two are very blinding and I am old.
In fact... I'm a bit tempted to not have documentation for this because I feel like its too small and specific feature. What do the rest of the reviewers think?
@kaixin-hckaixin-hcApr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one is probably good enough! But the first one is not obvious enough - maybe play with the colours a bit so it stands out more.
Screenshot 2024-04-06 at 8 11 17 PM
I'm team documentation here, the use case becomes far clearer when you see how the seamless panel looks inside a colored background.
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep I just left one example on the user guide so that user is at least aware of this property of seamless panels. Please tell me if the colour combo is not nice 😅
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Colour looks good to me : )
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 margin-top: 5px;
 display: flex;
 align-items: center;
 color: inherit;
@yucheng11122017yucheng11122017Apr 6, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is the difference between .morph-display-wrapper-seamless and .morph-display-wrapper only the inheritance of color?

In that case, could we make it such that .morph-display-wrapper-seamless is just color: inherit? So that if we decide to change like the margin-top or something, we only change it in one place

So the button will always have the class .morph-display-wrapper and will have the class .morph-display-wrapper-seamless if it is seamless
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep sorry about that, I have made the changes accordingly
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I've changed it now so that both the seamless and non-seamless morph wrapper would share the same basic styles, and only morph-display-wrapper-seamless will have the color:inherit property. This allows for future changes to the styles of all morph wrappers to be done in one place and allows the seamless wrapper to have the same styles as the other wrappers
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

kaixin-hc
kaixin-hc
reviewed
Apr 6, 2024
View reviewed changes
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
kaixin-hc
kaixin-hc
reviewed
Apr 7, 2024
View reviewed changes
Comment threaddocs/userGuide/syntax/panels.md
Outdated


 :bulb: Seamless panels inherit the background colour and text colour of any parents!
 <br/>
 <panel type="seamless" header="This is an example seamless panel" popup-url="/userGuide/syntax/extra/loadContent.html">
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why the popup-URL ?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'm just making sure that all buttons follow the seamless behaviour when testing, I'll remove from the final example!
yucheng11122017
yucheng11122017
reviewed
Apr 9, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Small nit!
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 <button :class="['morph-display-wrapper', 'btn', btnType]" @click="open()">
 <button
 :class="[isSeamless ? 'morph-display-wrapper-seamless': 'morph-display-wrapper',
 'btn',
@yucheng11122017yucheng11122017Apr 9, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't this button has the morph-display-wrapper anyway regardless of whether it is seamless or not?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I added the new morph-display-wrapper-seamless since I've noticed that the colour of the minimized panels would not inherit the parent's colour with just morph-display-wrapper
With just morph-display-wrapper:
Screen.Recording.2024-04-09.at.11.30.27.PM.mov
With morph-display-wrapper-seamless and morph-display-wrapper
Screen.Recording.2024-04-09.at.11.32.06.PM.mov
@yucheng11122017yucheng11122017Apr 11, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

As in does the a seamless button also need the properties from morph-display-wrapper?
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No seamless buttons wouldn't need properties from morph-display-wrapper, seamless buttons only appear when the panel is not minimised so it shouldn't need any properties from morph-display-wrapper
@kaixin-hckaixin-hcApr 12, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I've taken a video for your reference that shows that toggling these properties would make a visual difference. The caret button seems to also be a seamless button which appears when the panel is minimised, so the non seamless panels and the seamless panels then have slightly different styling due to this line of code.
Screen.Recording.2024-04-12.at.7.54.19.PM.mov
@yucheng11122017yucheng11122017Apr 16, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Rather than doing this or else statement, could all buttons have morph-display-wrapper by default and then only seamless buttons will have morph-display-wrapper-seamless?
@jingting1412jingting1412Apr 16, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

yep sure I've implemented it as such now
yucheng11122017
yucheng11122017
approved these changes
Apr 11, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM except for one small clarification!
yucheng11122017
yucheng11122017
approved these changes
Apr 16, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@yiwen101
yiwen101
merged commit 8253f82
into
MarkBind:masterApr 17, 2024
@yiwen101yiwen101
added
the 
r.Patch
Version resolver: increment by 0.0.1
label
Apr 17, 2024
itsyme pushed a commit
that referenced
this pull request
Apr 19, 2024
@jingting1412@yucheng11122017
@kaixin-hc@itsyme
Enable seamless panels to inherit any parent's colours (#2501)
8063cdb
* Change seamless panels
* Improve look of seamless panels
* Add tests
* Update userguide example
---------
Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
Co-authored-by: Hannah <59471320+kaixin-hc@users.noreply.github.com>
@damithc
damithc
commented
May 3, 2024

Copy link


Copy Markdown

Contributor
Feature in action https://nus-cs2103-ay2324s2.github.io/website/schedule/week8/topics.html
image
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kaixin-hckaixin-hckaixin-hc left review comments
@yucheng11122017yucheng11122017yucheng11122017 approved these changes

Assignees


No one assigned


Labels

r.PatchVersion resolver: increment by 0.0.1

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jingting1412@kaixin-hc@damithc@yucheng11122017@yiwen101

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Change seamless panels to inherit any parent's colours by jingting1412 · Pull Request #2501 · MarkBind/markbind · GitHub
Skip to content
Change seamless panels to inherit any parent's colours - #2501
Merged
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Apr 17, 2024
Merged
Change seamless panels to inherit any parent's colours#2501
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Conversation
@jingting1412
@jingting1412jingting1412
commented
Apr 5, 2024•

edited by kaixin-hc

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request?
  •  Documentation update
  •  Bug fix
  •  Feature addition or enhancement
  •  Code maintenance
  •  DevOps
  •  Improve developer experience
  •  Others, please explain:
Overview of changes:
#1697

Changed the seamless panels so that the background is transparent and it inherits the colours of any parents its contained in.
Anything you'd like to highlight/discuss:

As pointed out in the issue, this might break some edge cases (e.g. a seamless panel in a <box background-color="black" color="black"> box).
Testing instructions:

I've added some examples in the userguide to show the new look. markbind serve -d the docs to see the changes and try out different cases to see if it works or if there's any improvements.
Proposed commit message: (wrap lines at 72 characters)

Change seamless panels to inherit colours of any parents
Checklist: ☑️
  •  Updated the documentation for feature additions and enhancements
  •  Added tests for bug fixes or features
  •  Linked all related issues
  •  No unrelated changes 
Reviewer checklist:
Indicate the SEMVER impact of the PR:
  •  Major (when you make incompatible API changes)
  •  Minor (when you add functionality in a backward compatible manner)
  •  Patch (when you make backward compatible bug fixes)
At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.
Breaking change release note preparation (if applicable):
  • To be included in the release note for any feature that is made obsolete/breaking
Give a brief explanation note about:
  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).
@codecov
codecovBot
commented
Apr 5, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 51.02%. Comparing base (fc776ba) to head (a721356).
❗ Current head a721356 differs from pull request most recent head b075167. Consider uploading reports for the commit b075167 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## master #2501 +/- ##
==========================================
+ Coverage 50.99% 51.02% +0.02% 
==========================================
Files 124 124 Lines 5383 5372 -11 Branches 1160 1159 -1 ==========================================
- Hits 2745 2741 -4 + Misses 2348 2341 -7 
Partials 290 290 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@jingting1412
jingting1412
commented
Apr 5, 2024

Copy link


Copy Markdown

ContributorAuthor
Idk how my other allocate space for scrollbar commit got here 😞 but the changed files only reflect the changes relevant to this pr!
@kaixin-hc
kaixin-hc
commented
Apr 6, 2024

Copy link


Copy Markdown

Contributor
Weird that the netlify deploy preview failed. Maybe it will fix itself on push
yucheng11122017
yucheng11122017
reviewed
Apr 6, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Hi @jingting1412 thanks for the work!

Functionality and all looks great - just some minor nits.
Could you also add some test cases for this behaviour? I think you should be able to add unit tests without too much issue
Comment threaddocs/userGuide/syntax/panels.md
Outdated

</variable>
</include>

<box>
@yucheng11122017yucheng11122017Apr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

TBH i think this is too much documentation and examples.. I would rather just keep one, preferably the first because the other two are very blinding and I am old.
In fact... I'm a bit tempted to not have documentation for this because I feel like its too small and specific feature. What do the rest of the reviewers think?
@kaixin-hckaixin-hcApr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one is probably good enough! But the first one is not obvious enough - maybe play with the colours a bit so it stands out more.
Screenshot 2024-04-06 at 8 11 17 PM
I'm team documentation here, the use case becomes far clearer when you see how the seamless panel looks inside a colored background.
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep I just left one example on the user guide so that user is at least aware of this property of seamless panels. Please tell me if the colour combo is not nice 😅
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Colour looks good to me : )
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 margin-top: 5px;
 display: flex;
 align-items: center;
 color: inherit;
@yucheng11122017yucheng11122017Apr 6, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is the difference between .morph-display-wrapper-seamless and .morph-display-wrapper only the inheritance of color?

In that case, could we make it such that .morph-display-wrapper-seamless is just color: inherit? So that if we decide to change like the margin-top or something, we only change it in one place

So the button will always have the class .morph-display-wrapper and will have the class .morph-display-wrapper-seamless if it is seamless
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep sorry about that, I have made the changes accordingly
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I've changed it now so that both the seamless and non-seamless morph wrapper would share the same basic styles, and only morph-display-wrapper-seamless will have the color:inherit property. This allows for future changes to the styles of all morph wrappers to be done in one place and allows the seamless wrapper to have the same styles as the other wrappers
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

kaixin-hc
kaixin-hc
reviewed
Apr 6, 2024
View reviewed changes
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
kaixin-hc
kaixin-hc
reviewed
Apr 7, 2024
View reviewed changes
Comment threaddocs/userGuide/syntax/panels.md
Outdated


 :bulb: Seamless panels inherit the background colour and text colour of any parents!
 <br/>
 <panel type="seamless" header="This is an example seamless panel" popup-url="/userGuide/syntax/extra/loadContent.html">
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why the popup-URL ?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'm just making sure that all buttons follow the seamless behaviour when testing, I'll remove from the final example!
yucheng11122017
yucheng11122017
reviewed
Apr 9, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Small nit!
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 <button :class="['morph-display-wrapper', 'btn', btnType]" @click="open()">
 <button
 :class="[isSeamless ? 'morph-display-wrapper-seamless': 'morph-display-wrapper',
 'btn',
@yucheng11122017yucheng11122017Apr 9, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't this button has the morph-display-wrapper anyway regardless of whether it is seamless or not?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I added the new morph-display-wrapper-seamless since I've noticed that the colour of the minimized panels would not inherit the parent's colour with just morph-display-wrapper
With just morph-display-wrapper:
Screen.Recording.2024-04-09.at.11.30.27.PM.mov
With morph-display-wrapper-seamless and morph-display-wrapper
Screen.Recording.2024-04-09.at.11.32.06.PM.mov
@yucheng11122017yucheng11122017Apr 11, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

As in does the a seamless button also need the properties from morph-display-wrapper?
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No seamless buttons wouldn't need properties from morph-display-wrapper, seamless buttons only appear when the panel is not minimised so it shouldn't need any properties from morph-display-wrapper
@kaixin-hckaixin-hcApr 12, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I've taken a video for your reference that shows that toggling these properties would make a visual difference. The caret button seems to also be a seamless button which appears when the panel is minimised, so the non seamless panels and the seamless panels then have slightly different styling due to this line of code.
Screen.Recording.2024-04-12.at.7.54.19.PM.mov
@yucheng11122017yucheng11122017Apr 16, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Rather than doing this or else statement, could all buttons have morph-display-wrapper by default and then only seamless buttons will have morph-display-wrapper-seamless?
@jingting1412jingting1412Apr 16, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

yep sure I've implemented it as such now
yucheng11122017
yucheng11122017
approved these changes
Apr 11, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM except for one small clarification!
yucheng11122017
yucheng11122017
approved these changes
Apr 16, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@yiwen101
yiwen101
merged commit 8253f82
into
MarkBind:masterApr 17, 2024
@yiwen101yiwen101
added
the 
r.Patch
Version resolver: increment by 0.0.1
label
Apr 17, 2024
itsyme pushed a commit
that referenced
this pull request
Apr 19, 2024
@jingting1412@yucheng11122017
@kaixin-hc@itsyme
Enable seamless panels to inherit any parent's colours (#2501)
8063cdb
* Change seamless panels
* Improve look of seamless panels
* Add tests
* Update userguide example
---------
Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
Co-authored-by: Hannah <59471320+kaixin-hc@users.noreply.github.com>
@damithc
damithc
commented
May 3, 2024

Copy link


Copy Markdown

Contributor
Feature in action https://nus-cs2103-ay2324s2.github.io/website/schedule/week8/topics.html
image
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kaixin-hckaixin-hckaixin-hc left review comments
@yucheng11122017yucheng11122017yucheng11122017 approved these changes

Assignees


No one assigned


Labels

r.PatchVersion resolver: increment by 0.0.1

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jingting1412@kaixin-hc@damithc@yucheng11122017@yiwen101

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
Change seamless panels to inherit any parent's colours by jingting1412 · Pull Request #2501 · MarkBind/markbind · GitHub
Skip to content
Change seamless panels to inherit any parent's colours - #2501
Merged
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Apr 17, 2024
Merged
Change seamless panels to inherit any parent's colours#2501
yiwen101 merged 24 commits into
MarkBind:masterfrom 
jingting1412:change-seamless
Conversation
@jingting1412
@jingting1412jingting1412
commented
Apr 5, 2024•

edited by kaixin-hc

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request?
  •  Documentation update
  •  Bug fix
  •  Feature addition or enhancement
  •  Code maintenance
  •  DevOps
  •  Improve developer experience
  •  Others, please explain:
Overview of changes:
#1697

Changed the seamless panels so that the background is transparent and it inherits the colours of any parents its contained in.
Anything you'd like to highlight/discuss:

As pointed out in the issue, this might break some edge cases (e.g. a seamless panel in a <box background-color="black" color="black"> box).
Testing instructions:

I've added some examples in the userguide to show the new look. markbind serve -d the docs to see the changes and try out different cases to see if it works or if there's any improvements.
Proposed commit message: (wrap lines at 72 characters)

Change seamless panels to inherit colours of any parents
Checklist: ☑️
  •  Updated the documentation for feature additions and enhancements
  •  Added tests for bug fixes or features
  •  Linked all related issues
  •  No unrelated changes 
Reviewer checklist:
Indicate the SEMVER impact of the PR:
  •  Major (when you make incompatible API changes)
  •  Minor (when you add functionality in a backward compatible manner)
  •  Patch (when you make backward compatible bug fixes)
At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.
Breaking change release note preparation (if applicable):
  • To be included in the release note for any feature that is made obsolete/breaking
Give a brief explanation note about:
  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).
@codecov
codecovBot
commented
Apr 5, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 51.02%. Comparing base (fc776ba) to head (a721356).
❗ Current head a721356 differs from pull request most recent head b075167. Consider uploading reports for the commit b075167 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@## master #2501 +/- ##
==========================================
+ Coverage 50.99% 51.02% +0.02% 
==========================================
Files 124 124 Lines 5383 5372 -11 Branches 1160 1159 -1 ==========================================
- Hits 2745 2741 -4 + Misses 2348 2341 -7 
Partials 290 290 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@jingting1412
jingting1412
commented
Apr 5, 2024

Copy link


Copy Markdown

ContributorAuthor
Idk how my other allocate space for scrollbar commit got here 😞 but the changed files only reflect the changes relevant to this pr!
@kaixin-hc
kaixin-hc
commented
Apr 6, 2024

Copy link


Copy Markdown

Contributor
Weird that the netlify deploy preview failed. Maybe it will fix itself on push
yucheng11122017
yucheng11122017
reviewed
Apr 6, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Hi @jingting1412 thanks for the work!

Functionality and all looks great - just some minor nits.
Could you also add some test cases for this behaviour? I think you should be able to add unit tests without too much issue
Comment threaddocs/userGuide/syntax/panels.md
Outdated

</variable>
</include>

<box>
@yucheng11122017yucheng11122017Apr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

TBH i think this is too much documentation and examples.. I would rather just keep one, preferably the first because the other two are very blinding and I am old.
In fact... I'm a bit tempted to not have documentation for this because I feel like its too small and specific feature. What do the rest of the reviewers think?
@kaixin-hckaixin-hcApr 6, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just one is probably good enough! But the first one is not obvious enough - maybe play with the colours a bit so it stands out more.
Screenshot 2024-04-06 at 8 11 17 PM
I'm team documentation here, the use case becomes far clearer when you see how the seamless panel looks inside a colored background.
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep I just left one example on the user guide so that user is at least aware of this property of seamless panels. Please tell me if the colour combo is not nice 😅
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Colour looks good to me : )
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 margin-top: 5px;
 display: flex;
 align-items: center;
 color: inherit;
@yucheng11122017yucheng11122017Apr 6, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Is the difference between .morph-display-wrapper-seamless and .morph-display-wrapper only the inheritance of color?

In that case, could we make it such that .morph-display-wrapper-seamless is just color: inherit? So that if we decide to change like the margin-top or something, we only change it in one place

So the button will always have the class .morph-display-wrapper and will have the class .morph-display-wrapper-seamless if it is seamless
@jingting1412jingting1412Apr 7, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yep sorry about that, I have made the changes accordingly
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I've changed it now so that both the seamless and non-seamless morph wrapper would share the same basic styles, and only morph-display-wrapper-seamless will have the color:inherit property. This allows for future changes to the styles of all morph wrappers to be done in one place and allows the seamless wrapper to have the same styles as the other wrappers
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

Comment threadpackages/vue-components/src/panels/NestedPanel.vue
Outdated

kaixin-hc
kaixin-hc
reviewed
Apr 6, 2024
View reviewed changes
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
kaixin-hc
kaixin-hc
reviewed
Apr 7, 2024
View reviewed changes
Comment threaddocs/userGuide/syntax/panels.md
Outdated


 :bulb: Seamless panels inherit the background colour and text colour of any parents!
 <br/>
 <panel type="seamless" header="This is an example seamless panel" popup-url="/userGuide/syntax/extra/loadContent.html">
@kaixin-hckaixin-hcApr 7, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Why the popup-URL ?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'm just making sure that all buttons follow the seamless behaviour when testing, I'll remove from the final example!
yucheng11122017
yucheng11122017
reviewed
Apr 9, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Small nit!
Comment threadpackages/vue-components/src/panels/NestedPanel.vue
 <button :class="['morph-display-wrapper', 'btn', btnType]" @click="open()">
 <button
 :class="[isSeamless ? 'morph-display-wrapper-seamless': 'morph-display-wrapper',
 'btn',
@yucheng11122017yucheng11122017Apr 9, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Shouldn't this button has the morph-display-wrapper anyway regardless of whether it is seamless or not?
@jingting1412jingting1412Apr 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I added the new morph-display-wrapper-seamless since I've noticed that the colour of the minimized panels would not inherit the parent's colour with just morph-display-wrapper
With just morph-display-wrapper:
Screen.Recording.2024-04-09.at.11.30.27.PM.mov
With morph-display-wrapper-seamless and morph-display-wrapper
Screen.Recording.2024-04-09.at.11.32.06.PM.mov
@yucheng11122017yucheng11122017Apr 11, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

As in does the a seamless button also need the properties from morph-display-wrapper?
@jingting1412jingting1412Apr 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

No seamless buttons wouldn't need properties from morph-display-wrapper, seamless buttons only appear when the panel is not minimised so it shouldn't need any properties from morph-display-wrapper
@kaixin-hckaixin-hcApr 12, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I've taken a video for your reference that shows that toggling these properties would make a visual difference. The caret button seems to also be a seamless button which appears when the panel is minimised, so the non seamless panels and the seamless panels then have slightly different styling due to this line of code.
Screen.Recording.2024-04-12.at.7.54.19.PM.mov
@yucheng11122017yucheng11122017Apr 16, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Rather than doing this or else statement, could all buttons have morph-display-wrapper by default and then only seamless buttons will have morph-display-wrapper-seamless?
@jingting1412jingting1412Apr 16, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

yep sure I've implemented it as such now
yucheng11122017
yucheng11122017
approved these changes
Apr 11, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM except for one small clarification!
yucheng11122017
yucheng11122017
approved these changes
Apr 16, 2024
View reviewed changes
@yucheng11122017yucheng11122017
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@yiwen101
yiwen101
merged commit 8253f82
into
MarkBind:masterApr 17, 2024
@yiwen101yiwen101
added
the 
r.Patch
Version resolver: increment by 0.0.1
label
Apr 17, 2024
itsyme pushed a commit
that referenced
this pull request
Apr 19, 2024
@jingting1412@yucheng11122017
@kaixin-hc@itsyme
Enable seamless panels to inherit any parent's colours (#2501)
8063cdb
* Change seamless panels
* Improve look of seamless panels
* Add tests
* Update userguide example
---------
Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
Co-authored-by: Hannah <59471320+kaixin-hc@users.noreply.github.com>
@damithc
damithc
commented
May 3, 2024

Copy link


Copy Markdown

Contributor
Feature in action https://nus-cs2103-ay2324s2.github.io/website/schedule/week8/topics.html
image
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kaixin-hckaixin-hckaixin-hc left review comments
@yucheng11122017yucheng11122017yucheng11122017 approved these changes

Assignees


No one assigned


Labels

r.PatchVersion resolver: increment by 0.0.1

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jingting1412@kaixin-hc@damithc@yucheng11122017@yiwen101