Stampy redesign 368 #412

buddy-web3 · 2024-02-16T10:20:45Z

No description provided.

mruwnik · 2024-02-16T17:21:50Z

app/components/Button/button.css

+
+.action-feedback-text {
+  display: none;
+  position: absolute;


can you do this with relative values? This will not work on smaller screens

mruwnik · 2024-02-16T17:24:17Z

app/components/Article/index.tsx

+            onSuccess={() => setShowFeedbackForm(true)}
+          />
+          <span className={['action-feedback-text', showFeedback ? 'show' : ''].join(' ')}>
+            Thanks for your feedback!


@melissasamworth what will cause this to be hidden again? Should it disappear e.g. after 10s? Otherwise it will hang around and be annoying

Yeah that works, disappears after 10s

this also should probably go into the feedback form

mruwnik · 2024-02-16T17:28:13Z

app/components/Article/FeedbackForm/index.tsx

+  }
+
+  return (
+    <div className={className}>


why not just make this into a html form? A radio input + button should handle most of this

all other comments have been addressed. this one is slightly more css-nasty.

mruwnik · 2024-02-16T17:28:25Z

app/components/Article/FeedbackForm/index.tsx

+          className={'feedback-text'}
+          placeholder={'Leave a comment (optional)'}
+        ></textarea>
+        <div className={['submit-feedback', enabledSubmit ? 'enabled' : ''].join(' ')}>


use the Button component

melissasamworth · 2024-02-16T20:29:30Z

app/components/Article/FeedbackForm/feedbackForm.css

+  border-radius: 6px;
+  background-color: #fff;
+  box-shadow: 0px 16px 40px rgba(175, 183, 194, 0.2);
+  border: 1px solid #dfe3e9;


could you reference the var?

melissasamworth · 2024-02-16T20:30:42Z

app/components/Article/FeedbackForm/feedbackForm.css

+  border-radius: 6px;
+  background-color: #fff;
+  box-shadow: 0px 16px 40px rgba(175, 183, 194, 0.2);
+  border: 1px solid #dfe3e9;


here too

As a general rule, too, if we're repeating code a lot we should just create a one-off class for newroot.css like

.border {
border: 1px solid #dfe3e9;
}

melissasamworth · 2024-02-16T20:32:52Z

app/components/Article/FeedbackForm/feedbackForm.css

+  position: relative;
+  height: 542px;
+  text-align: left;
+  font-size: 16px;


The idea was to make it so you never have to write a line of code related to anything type. Just look at what class I use in Figma and find that same class in newroot.css

melissasamworth · 2024-02-16T20:33:12Z

app/components/Article/FeedbackForm/feedbackForm.css

+  text-align: left;
+  font-size: 16px;
+  color: var(--colors-cool-grey-600);
+  font-family: Poppins;


same as comment above :)

melissasamworth · 2024-02-16T20:33:41Z

app/components/Article/FeedbackForm/feedbackForm.css

+}
+.small {
+  position: relative;
+  letter-spacing: -0.01em;


same again heh

melissasamworth · 2024-02-16T20:33:45Z

app/components/Article/FeedbackForm/feedbackForm.css

+.small {
+  position: relative;
+  letter-spacing: -0.01em;
+  line-height: 170%;


melissasamworth · 2024-02-16T20:33:51Z

app/components/Article/FeedbackForm/feedbackForm.css

+.small3 {
+  position: relative;
+  letter-spacing: -0.01em;
+  line-height: 170%;


melissasamworth · 2024-02-16T20:33:56Z

app/components/Article/FeedbackForm/feedbackForm.css

+  position: relative;
+  letter-spacing: -0.01em;
+  line-height: 170%;
+  font-weight: 600;


melissasamworth · 2024-02-16T20:34:37Z

app/components/Article/FeedbackForm/feedbackForm.css

+  flex-direction: row;
+  align-items: center;
+  justify-content: center;
+  padding: 7px 17px;


Always use a variable. If the padding in the file was 7px I meant for it to be 8px sorry! 17px would be 16px.

melissasamworth · 2024-02-16T20:36:05Z

app/components/Article/index.tsx

+            onSuccess={() => setShowFeedbackForm(true)}
+          />
+          <span className={['action-feedback-text', showFeedback ? 'show' : ''].join(' ')}>
+            Thanks for your feedback!


Yeah that works, disappears after 10s

mruwnik · 2024-02-20T09:33:06Z

app/components/Button/index.tsx

 }
-const Button = ({children, action, tooltip, icon, className}: ButtonProps) => {
+const Button = ({children, action, tooltip, icon, className, disabled = false}: ButtonProps) => {


how would this work for link buttons? You can add a onClick={(e) => e.preventDefault()} or something, to disable it, but it might require a bit extra styling

is it necessary for anything else besides the feedback form?

not now, but it would be good to have it done for consistency

mruwnik · 2024-02-20T09:33:46Z

app/components/Button/button.css

@@ -107,3 +107,34 @@
 .tooltip:hover::after {
  display: block;
 }
+


could you move the feedback specific items to feedbackForm.css?

mruwnik · 2024-02-20T09:40:24Z

app/components/Article/FeedbackForm/index.tsx

+        <span className={'feedback-header'}>What was the problem?</span>
+        {hasOptions
+          ? feedbackOptions.map((option, index) => (
+              <div


aren't these also buttons? So something like <Button className={option.selected ? 'secondary-alt bordered' : 'secondary'} action={() => selectFeedback(index)}>{option.text}</Button>

mruwnik · 2024-02-20T09:44:42Z

app/components/Article/FeedbackForm/index.tsx

+  ])
+  const [enabledSubmit, setEnabledSubmit] = React.useState(false)
+  const selectFeedback = (option: number) => {
+    setFeedbackOptions(


why not just store the selected item? So:

const [selected, setSelected] = useState<string>() const selectFeedback = (option: string) => { setSelected(option) } (...) <div className={option.option === selected ? 'selected' : ''} onClick={() => selectFeedback(option.option)}> {option.text} </div>

mruwnik · 2024-02-20T09:47:29Z

app/components/Article/index.tsx

  const date =
    question.updatedAt &&
    new Date(question.updatedAt).toLocaleDateString('en-GB', {
      year: 'numeric',
      month: 'short',
    })

+  React.useEffect(() => {


why not move all this stuff into the feedback form component? It's only hidden, so the HTML elements will still be around

for storybook functionality

that's the wrong way round. Storybook, tests etc. shouldn't change how the actual code is structured. If something is hard to do without changes, then it's a hint that something is wrong, but the code should dictate storybook, not the other way round

You can add extra wrappers with decorators - check the ArticlesKeepGoing story

I've been looking into this but I don't believe it's a feasible option. The article makes it display when submitted, moving part of the logic to the Feedback component would just duplicate logic and code.

mruwnik · 2024-02-20T09:47:56Z

app/components/Article/index.tsx

+            onSuccess={() => setShowFeedbackForm(true)}
+          />
+          <span className={['action-feedback-text', showFeedback ? 'show' : ''].join(' ')}>
+            Thanks for your feedback!


this also should probably go into the feedback form

mruwnik · 2024-02-20T09:48:30Z

app/components/Article/FeedbackForm/feedbackForm.css

@@ -0,0 +1,44 @@
+.feedback-header {
+  color: var(--colors-black);


you can just use the black class for this

mruwnik

Code looks ok, @melissasamworth final approve is up to you if the CSS is acceptable

melissasamworth · 2024-02-22T19:23:13Z

app/components/Article/FeedbackForm/feedbackForm.css

@@ -0,0 +1,63 @@
+.feedback-container {
+  width: 384px;


what is feedback container? We'll want to be added .col-* class to different elements instead of defining the width through writing more CSS. I imagine this might be one of those cases? :)

melissasamworth · 2024-02-22T19:24:38Z

app/components/Article/FeedbackForm/feedbackForm.css

+.feedback-form {
+  width: 100%;
+  position: relative;
+  text-align: left;


isn't text-align left by default? If not we should define this in the body tag, so we don't have to define it again here

melissasamworth · 2024-02-22T19:25:07Z

app/components/Article/FeedbackForm/feedbackForm.css

+  width: 100%;
+  position: relative;
+  text-align: left;
+  font-size: 16px;


We don't want to ever write CSS related to typography, just reference whichever class was assigned to it in Figma!

melissasamworth · 2024-02-22T19:26:19Z

app/components/Article/FeedbackForm/feedbackForm.css

+.action-feedback-text {
+  display: none;
+  position: absolute;
+  left: 180px;


I'm skeptical about how these kinds of things will convert to mobile but let's see what happens

melissasamworth · 2024-02-22T20:21:29Z

app/components/Button/button.css

-  display: block;
+.button.full-width {
+  width: 100%;
+  height: 48px;


I would say use the button component and apply the relevant class

melissasamworth · 2024-02-22T20:21:43Z

app/components/Button/button.css

+  height: 48px;
+  border: 0;
+}
+.button:disabled,


these kinds of things should be added to the button component

buddy-web3 added 2 commits February 16, 2024 12:18

new component, styling fixes, visual feedback

413d86d

new component, styling fixes, visual feedback

922388b

buddy-web3 requested review from melissasamworth, Aprillion and mruwnik February 16, 2024 10:20

buddy-web3 added 3 commits February 16, 2024 12:22

eslint

34d98c2

eslint

f4bfbda

eslint

9664932

Aprillion removed their request for review February 16, 2024 10:47

mruwnik reviewed Feb 16, 2024

View reviewed changes

melissasamworth requested changes Feb 16, 2024

View reviewed changes

as per comments + new Figma feedback form

d907169

buddy-web3 mentioned this pull request Feb 20, 2024

bottom bits on standard article page #368

Closed

mruwnik reviewed Feb 20, 2024

View reviewed changes

requested changes

4073ce8

mruwnik reviewed Feb 21, 2024

View reviewed changes

melissasamworth requested changes Feb 22, 2024

View reviewed changes

buddy-web3 and others added 2 commits February 27, 2024 04:47

design requests

3f16217

Merge branch 'stampy-redesign' into stampy-redesign-368

dcba680

melissasamworth approved these changes Mar 6, 2024

View reviewed changes

melissasamworth merged commit 3942184 into stampy-redesign Mar 6, 2024
1 check passed

melissasamworth deleted the stampy-redesign-368 branch March 6, 2024 01:03

		@@ -0,0 +1,44 @@
		.feedback-header {
		color: var(--colors-black);

Stampy redesign 368 #412

Stampy redesign 368 #412

Conversation

buddy-web3 commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruwnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment